[NameLookup] Swift 4 compatibility hack for cross-module var overloads in generic type extensions #18488

hamishknight · 2018-08-03T14:58:35Z

In Swift 4, we only gave custom overload types to properties defined in extensions of generic types, using the null type for any other var decl. This meant that a property defined in such an extension would never shadow a property not defined in such an extension. As a result, this permitted cross-module overloads of properties of different types on generic types in certain cases.

#15412 changed the overload type logic to produce more consistent overload types, but in doing so inadvertently caused a source breakage by rejecting such cross-module property overloads (as we now always consider the property in the current module to shadow the imported one).

This PR adds an exception to the shadowing rules for properties defined in generic type extensions under Swift 4 mode. Permitting cross-module property overloads in the general case would be source breaking (causing ambiguity errors), so this can be handled in a follow-up Swift 5 mode PR if desired.

In addition, this PR adds a test case for an AnyObject lookup ambiguity error introduced by #15412.

Resolves SR-7341.

hamishknight · 2018-08-03T14:59:38Z

cc. @jrose-apple & @DougGregor. I know it's pretty late in the release cycle, but would it be possible to cherry pick this to 4.2?

DougGregor

This change LGTM!

DougGregor · 2018-08-03T17:53:54Z

I think we've missed the window for Swift 4.2, particularly with changes that could affect source compatibility. This looks good for master, though, thanks!

jrose-apple · 2018-08-03T17:56:09Z

@swift-ci Please test

swift-ci · 2018-08-03T18:22:56Z

Build failed
Swift Test OS X Platform
Git Sha - bac01cf400ba6cc541789cfd792601638fef7ebe

hamishknight · 2018-08-03T18:41:44Z

I think we've missed the window for Swift 4.2

That's what I feared – thanks for confirming!

Can you confirm whether or not we want cross-module property overloading in the general case for Swift 5 mode (bearing in mind it being potentially source-breaking)? If so, I'm happy to work on that as a follow-up.

hamishknight · 2018-08-03T18:44:20Z

Also looks like the OS X buildbot is failing consistently with:

invalid argument '-mmacosx-version-min=10.9' not allowed with '-mtvos-simulator-version-min=9.0'

swift-ci · 2018-08-03T19:58:40Z

Build failed
Swift Test Linux Platform
Git Sha - bac01cf400ba6cc541789cfd792601638fef7ebe

jrose-apple · 2018-08-03T20:07:33Z

cc @clackary, @shahmishal

shahmishal · 2018-08-03T20:35:26Z

This issue should be resolved, we reverted the commit which caused the issue.

shahmishal · 2018-08-03T20:35:39Z

@swift-ci test

slavapestov · 2018-08-07T05:44:32Z

@hamishknight Maybe this warrants some evolution discussion, if not a full proposal just a thread. I'd like to understand the new rules in 4.2 as well as the compatibility hacks you added to remain compatible with 4.1, and come up with a model that's simple and consistent. It's OK to have minor source breaks in -swift-version 5 mode; we just have to ensure the new behavior makes the most sense going forward.

slavapestov · 2018-08-07T05:44:58Z

lib/AST/NameLookup.cpp

+        // This is due to the fact that in Swift 4, we only gave custom overload
+        // types to properties in extensions of generic types, otherwise we
+        // used the null type.
+        if (!ctx.isSwiftVersionAtLeast(5)) {


What if we instead said that in Swift 5, all properties get a real overload signature? Why should properties be any different from methods in this regard?

@slavapestov By "real overload signature", do you mean that two properties of different type would always be allowed to overload each other? I'm not mad on this idea, as IMO it would encourage tricky code that relies on overload resolution by "return type", which I believe is something we want to discourage (listed in the API design guidelines under "General conventions" > "Methods can share a base name").

However given that we already allow what would otherwise be re-declarations across module boundaries for both properties and methods, I personally think that it seems reasonable to allow different typed properties to overload across modules (as the shadowing behaviour is otherwise a bit unintuitive).

Though whatever we choose, I agree we should definitely make the behaviour consistent in Swift 5 mode!

hamishknight · 2018-08-07T15:07:34Z

@slavapestov Thanks for the suggestion! I'm happy to open to an evolution thread to discuss this change (and the more general behaviour going forwards) in more detail – I'll try and do so before the end of the day.

hamishknight · 2018-08-08T13:54:21Z

Opened an evolution thread: https://forums.swift.org/t/should-we-allow-cross-module-overloading-of-variables-and-properties/15096

hamishknight · 2018-08-15T11:24:19Z

Just resolved a merge conflict – is this good to merge? I'll file a JIRA to track the Swift 5 mode changes.

slavapestov

Looks good, can you fix the test case and rebase and then we'll merge?

slavapestov · 2018-08-15T21:51:26Z

test/NameBinding/import-resolution-overload_swift5.swift

+
+func useString(_ str: String) {}
+
+// In Swift 5, we currently consistently reject cross-module property overloading.


You should have test errors that expect errors in that case

Thanks Slava, I've gone ahead and made that change.

Did you not push it yet?

I did push, and it shows up on GitHub for me (HEAD at c54ab4d). Should I re-rebase and try to push again?

I'm looking at this part:

// In Swift 5, we currently consistently reject cross-module property overloading. // FIX-ME: This seems reasonable to permit.

Is there a test for this behavior?

@slavapestov Apologies, I realise now that the comment isn't particularly well-worded. When I say "reject cross-module property overloading", what I really mean is "properties from this module always shadow properties from the other module, and therefore the other module's properties never show up in the overload set" – which is what the tests in that file are testing.

Your original comment prompted me to add expected-errors in order to ensure the other module's properties didn't show up in the overload set (previously I was just testing that a bare reference wasn't ambiguous).

I'll edit the comment in the test file to clarify things.

…s in generic type extensions In Swift 4, we only gave custom overload types to properties defined in extensions of generic types, using the null type for any other var decl. This meant that a property defined in such an extension would never shadow a property not defined in such an extension. As a result, this permitted cross-module overloads of properties of different types on generic types in certain cases. This patch adds an exception to the shadowing rules for properties defined in generic type extensions under Swift 4 mode. Permitting cross-module property overloads in the general case would also be source breaking (causing ambiguity errors), so this can be handled in a follow-up Swift 5 mode PR if desired. Resolves SR-7341.

slavapestov · 2018-08-20T21:14:28Z

@swift-ci Please test

slavapestov · 2018-08-20T21:14:35Z

@swift-ci Please test source compatibility

swift-ci · 2018-08-20T21:16:47Z

Build failed
Swift Test Linux Platform
Git Sha - bac01cf400ba6cc541789cfd792601638fef7ebe

swift-ci · 2018-08-20T21:18:03Z

Build failed
Swift Test OS X Platform
Git Sha - bac01cf400ba6cc541789cfd792601638fef7ebe

hamishknight · 2018-08-23T15:40:59Z

Thanks Slava! I've filed a JIRA for the Swift 5 mode changes: https://bugs.swift.org/browse/SR-8619.

hamishknight · 2018-09-06T22:06:24Z

@slavapestov Do you think this would be worth cherry picking to the 4.2 branch now that it's taking changes for a potential 4.2.1 release? IMO the risk should be minimal.

slavapestov · 2018-09-07T05:11:59Z

There's no harm in opening a PR; final decision rests with @tkremenek though.

hamishknight · 2018-09-10T21:10:00Z

Opened a 4.2 PR: #19223

DougGregor approved these changes Aug 3, 2018

View reviewed changes

slavapestov reviewed Aug 7, 2018

View reviewed changes

slavapestov approved these changes Aug 15, 2018

View reviewed changes

slavapestov self-assigned this Aug 15, 2018

Add test for dynamic member lookup behaviour changed by swiftlang#15412

2770632

hamishknight force-pushed the swift4-cross-module-var-overload branch 2 times, most recently from c54ab4d to be88c40 Compare August 18, 2018 11:30

hamishknight force-pushed the swift4-cross-module-var-overload branch from be88c40 to 1e764cc Compare August 18, 2018 11:35

slavapestov merged commit 98e95bd into swiftlang:master Aug 22, 2018

hamishknight deleted the swift4-cross-module-var-overload branch August 22, 2018 10:08

hamishknight restored the swift4-cross-module-var-overload branch September 9, 2018 18:42

hamishknight mentioned this pull request Sep 10, 2018

[4.2][NameLookup] Swift 4 compatibility hack for cross-module var overloads in generic type extensions #19223

Closed

hamishknight deleted the swift4-cross-module-var-overload branch September 10, 2018 21:10

This was referenced Aug 3, 2018

[SR-7341] Type checker regression for multiple properties with different return types as of March 28 master snapshot #49889

Closed

[SR-8704] Cast from Any? to as? Value where Value == Any produces unexpected result in Xcode 10 #51216

Closed


		func useString(_ str: String) {}

		// In Swift 5, we currently consistently reject cross-module property overloading.

[NameLookup] Swift 4 compatibility hack for cross-module var overloads in generic type extensions #18488

[NameLookup] Swift 4 compatibility hack for cross-module var overloads in generic type extensions #18488

Uh oh!

Conversation

hamishknight commented Aug 3, 2018

Uh oh!

hamishknight commented Aug 3, 2018

Uh oh!

DougGregor left a comment

Choose a reason for hiding this comment

Uh oh!

DougGregor commented Aug 3, 2018

Uh oh!

jrose-apple commented Aug 3, 2018

Uh oh!

swift-ci commented Aug 3, 2018

Uh oh!

hamishknight commented Aug 3, 2018

Uh oh!

hamishknight commented Aug 3, 2018

Uh oh!

swift-ci commented Aug 3, 2018

Uh oh!

jrose-apple commented Aug 3, 2018

Uh oh!

shahmishal commented Aug 3, 2018

Uh oh!

shahmishal commented Aug 3, 2018

Uh oh!

slavapestov commented Aug 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hamishknight commented Aug 7, 2018

Uh oh!

hamishknight commented Aug 8, 2018

Uh oh!

hamishknight commented Aug 15, 2018

Uh oh!

slavapestov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slavapestov commented Aug 20, 2018

Uh oh!

slavapestov commented Aug 20, 2018

Uh oh!

swift-ci commented Aug 20, 2018

Uh oh!

swift-ci commented Aug 20, 2018

Uh oh!

hamishknight commented Aug 23, 2018

Uh oh!

hamishknight commented Sep 6, 2018

Uh oh!

slavapestov commented Sep 7, 2018

Uh oh!

hamishknight commented Sep 10, 2018

Uh oh!

Uh oh!