SE-0036: Requiring Leading Dot Prefixes for Enum Instance Member Implementations #2224

ahoppen · 2016-04-17T10:12:32Z

What's in this pull request?

Implemented SE-0036: Requiring Leading Dot Prefixes for Enum Instance Member Implementations

Resolved bug number: SR-1236

Before merging this pull request to apple/swift repository:

Test pull request on Swift continuous integration.

Triggering Swift CI

The swift-ci is triggered by writing a comment on this PR addressed to the GitHub user @swift-ci. Different tests will run depending on the specific comment that you use. The currently available comments are:

Smoke Testing

Platform	Comment
All supported platforms	@swift-ci Please smoke test
OS X platform	@swift-ci Please smoke test OS X platform
Linux platform	@swift-ci Please smoke test Linux platform

Validation Testing

Platform	Comment
All supported platforms	@swift-ci Please test
OS X platform	@swift-ci Please test OS X platform
Linux platform	@swift-ci Please test Linux platform

Note: Only members of the Apple organization can trigger swift-ci.

ahoppen · 2016-04-17T10:23:34Z

I also submitted a pull request to swift-package-manager to update its code for the changes: #260

jrose-apple · 2016-04-20T17:08:52Z

Interesting. I'm only realizing now that SE-0036 isn't precise enough: does it mean enums have to be accessed as static members (this patch), or that enum cases should never be found via unqualified lookup, even in static methods?

Please add a test with a static method either way.

jrose-apple · 2016-04-20T17:10:24Z

Ah, I see that case is actually called out in the "Detail Design" section, and I see your test for it as well now. My bad.

jrose-apple · 2016-04-20T17:10:53Z

include/swift/AST/DiagnosticsSema.def

@@ -93,6 +93,9 @@ ERROR(could_not_use_type_member,none,
 ERROR(could_not_use_type_member_on_instance,none,
      "static member %1 cannot be used on instance of type %0",
      (Type, DeclName))
+ERROR(could_not_use_enum_element_on_instance,none,
+      "enum element %0 cannot be referenced as instance members",


Singular/plural mismatch here. How about "…as an instance member"?

Sounds good, I’ll change it

ahoppen · 2016-04-20T18:38:53Z

I updated the error message and added a fix-it to suggest the full type name. This changed the code in CSDiag.cpp, the error message in DiagnosticsSema.def and of course the test cases in test/Parse/enum.swift. Can you have a look again?

tkremenek · 2016-05-04T22:30:45Z

@jrose-apple can you take a look?

jrose-apple · 2016-05-04T23:19:53Z

lib/Sema/CSDiag.cpp

+              llvm::raw_string_ostream typeNameStream(enumTypeName);
+              typeNameStream << parentEnum->getNameStr();
+              if (auto genericParams = parentEnum->getGenericParams()) {
+                genericParams->print(typeNameStream);


Won't this include constraints and things we don't want? (Please add that to your test either way.)

Actually, in 99% of cases the generic parameters will be assumed anyway, so maybe it'd be better to just leave them out.

jrose-apple · 2016-05-04T23:31:55Z

Sorry for the delay!

Aside from my low-level comments, I think always adding the fully-qualified name is the wrong way to go. If a fix-it isn't code that most users would actually accept, we shouldn't offer it. If there's not an easy way to detect whether the fully-qualified name is the right way to go, or whether the single dot would work, we should just leave it out for now.

ahoppen · 2016-05-05T15:37:11Z

That was my initial concern as well. I can’t think of a way to determine if a single dot suffices since it depends on the parent of the expression that caused the error and I can’t think of a way to determine it. I just though it would be helpful for the migration assistant to offload the migration work to the IDE instead of the developer.
But since you think otherwise, I removed the fix-it again.

jrose-apple · 2016-05-05T15:53:49Z

We can always improve the diagnostic later if we think of something. This is looking good!

@swift-ci Please test

ahoppen · 2016-05-05T17:37:06Z

Sorry, missed one declaration in the standard library when resolving merge conflicts. Don’t know why my local tests didn’t catch it. Should be resolved now. Testing locally but am pretty sure it was the only case. Could you try again?

Update: Local test succeeded after change

jrose-apple · 2016-05-05T20:49:17Z

@swift-ci Please test

jrose-apple · 2016-05-05T22:48:18Z

Probably unrelated?

@swift-ci Please test Linux platform

DougGregor · 2016-05-06T03:36:21Z

@swift-ci Please test

…plementations

ahoppen · 2016-05-07T21:34:23Z

I didn’t know there were tests for the swift-pm that could fail because of this. I submitted a pull request for swift-pm to incorporate the changes: swiftlang/swift-package-manager#325

As to the failings on Linux, I’ve got no idea what caused them and don’t have a Linux machine to test it on, but it seems unrelated to my changes to me…

tkremenek · 2016-05-08T05:29:09Z

@swift-ci test

jrose-apple · 2016-05-11T02:57:31Z

I'm going to ignore the LLDB failures. Thanks, Alex!

bob-wilson · 2016-05-11T06:02:23Z

The LLDB failures are breaking a number of the Jenkins bots. Is someone looking into fixing those?

alexwlchan · 2016-05-11T06:38:27Z

I’ve been getting CI failure emails that I think are meant for @ahoppen, coming from this PR. And I haven’t touched Swift in a while, so I’ve just been deleting them.

That might be why nobody’s been looking into the failures.

bob-wilson · 2016-05-11T15:43:13Z

Todd Fiala just committed a fix for the LLDB failures. I'm following up on fixing the confusion between Alex Hoppen and Alex Chan.

jrose-apple · 2016-05-11T16:05:36Z

Ah, then I apologize for assuming they were unrelated! @tfiala, I'll be more careful in the future.

tfiala · 2016-05-11T16:10:13Z

No worries, @jrose-apple :-)

tkremenek · 2016-05-11T18:02:12Z

I am going to need to revert this. What I have found from running this change on various code is that there is no migration help. We provide no fixits for missing dots, case statements simply say "invalid pattern" instead of suggesting a dot, etc. With this kind of breaking change we need better migration support.

tkremenek · 2016-05-11T18:07:55Z

For example, for things like this:

 func staticReferencInInstanceMethod() {
    let _ = A // expected-error {{enum element 'A' cannot be referenced as an instance member}}
    let _ = SE0036.A
}

We should have a Fix-it, not just an error.

Also, for things like this:

switch something {
   case enumCase(let i):

Right now we get "invalid pattern". We need a fixit and a better error to help users move forward.

tkremenek · 2016-05-11T18:08:42Z

For reference, his was reverted in #2477.

tkremenek · 2016-05-11T18:09:21Z

@ahoppen I really appreciate you working on this. I know this has taken some iteration, but this just needs some more refinement before it can land. This became apparent when I saw the fallout from landing this change.

jrose-apple · 2016-05-11T20:48:58Z

Hm. We had a fix-it earlier, and found that it usually didn't match what users would naturally use: it inserted the full name of the enum rather than just a dot.

slavapestov · 2016-05-11T20:56:32Z

include/swift/AST/DiagnosticsSema.def

@@ -93,6 +93,9 @@ ERROR(could_not_use_type_member,none,
 ERROR(could_not_use_type_member_on_instance,none,
      "static member %1 cannot be used on instance of type %0",
      (Type, DeclName))
+ERROR(could_not_use_enum_element_on_instance,none,
+      "enum element %0 cannot be referenced as an instance member",


In fact I think it would be better if the diagnostic just said the leading dot is required

That only works if in a contextual position where a leading dot will work.

ahoppen · 2016-05-12T21:27:09Z

First of all a big thanks to @tfiala for fixing the issues that I caused in LLDB and sorry for all the trouble caused.

This weekend I will go and have a look whether I can come up with an idea on how the determine if a dot suffices in a fix-it that I didn’t think of before. I will report back then.

tfiala · 2016-05-12T21:51:35Z

On May 12, 2016, at 2:27 PM, Alex Hoppen [email protected] wrote:

First of all a big thanks to @tfiala https://github.com/tfiala for fixing the issues that I caused in LLDB and sorry for all the trouble caused.

No worries, Alex! It was fine.

I am going to work on improving exposure of build-related failures during an LLDB test run (I.e. when an LLDB test fails to compile a test subject, like a .swift, .cpp or .c program that will be used during testing). The intent will be to make this show up neatly in the Swift CI test output. That’ll go a long way to making it more obvious when a Swift change requires modifying some LLDB test subject .swift/.c/.cpp files.

This weekend I will go and have a look whether I can come up with an idea on how the determine if a dot suffices in a fix-it that I didn’t think of before. I will report back then.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub #2224 (comment)

ahoppen · 2016-05-21T10:38:51Z

I created an updated pull request with fix its: #2634

ahoppen mentioned this pull request Apr 17, 2016

Update for changes made in SE-0036 swiftlang/swift-package-manager#260

Merged

ahoppen force-pushed the SR-1236 branch 2 times, most recently from 0f2c3b1 to 2688e72 Compare April 17, 2016 15:10

jrose-apple reviewed Apr 20, 2016
View reviewed changes

ahoppen force-pushed the SR-1236 branch from 2688e72 to efd2ab1 Compare April 20, 2016 18:34

tkremenek assigned jrose-apple May 4, 2016

jrose-apple reviewed May 4, 2016
View reviewed changes

ahoppen force-pushed the SR-1236 branch 6 times, most recently from 2e9b1f9 to fd32b4b Compare May 5, 2016 15:31

ahoppen force-pushed the SR-1236 branch from fd32b4b to 2df0a43 Compare May 5, 2016 17:32

ahoppen added 3 commits May 7, 2016 22:39

[Gardening] Changed 0 to nullptr for pointer comparison

eaa26fa

[Gardening] Fixed formatting issue in ASTDumper

b7abfd2

[SE-0036]: Requiring Leading Dot Prefixes for Enum Instance Member Im…

0e8c69c

…plementations

ahoppen force-pushed the SR-1236 branch from 2df0a43 to 0e8c69c Compare May 7, 2016 21:30

jrose-apple merged commit 14a7334 into swiftlang:master May 11, 2016

tkremenek mentioned this pull request May 11, 2016

Revert "SE-0036: Requiring Leading Dot Prefixes for Enum Instance Member Implementations" #2477

Merged

slavapestov reviewed May 11, 2016
View reviewed changes

ahoppen mentioned this pull request May 21, 2016

SE-0036: Requiring Leading Dot Prefixes for Enum Instance Member Implementations (once again) #2634

Closed

1 task

ahoppen mentioned this pull request Jun 2, 2016

SE-0036 (1/3): Update existing tests for the upcoming implementation #2841

Merged

1 task

SE-0036: Requiring Leading Dot Prefixes for Enum Instance Member Implementations #2224

SE-0036: Requiring Leading Dot Prefixes for Enum Instance Member Implementations #2224

Uh oh!

Conversation

ahoppen commented Apr 17, 2016

What's in this pull request?

Resolved bug number: SR-1236

Uh oh!

ahoppen commented Apr 17, 2016

Uh oh!

jrose-apple commented Apr 20, 2016

Uh oh!

jrose-apple commented Apr 20, 2016

Uh oh!

jrose-apple Apr 20, 2016

Choose a reason for hiding this comment

Uh oh!

ahoppen Apr 20, 2016

Choose a reason for hiding this comment

Uh oh!

ahoppen commented Apr 20, 2016

Uh oh!

tkremenek commented May 4, 2016

Uh oh!

jrose-apple May 4, 2016

Choose a reason for hiding this comment

Uh oh!

jrose-apple commented May 4, 2016

Uh oh!

ahoppen commented May 5, 2016

Uh oh!

jrose-apple commented May 5, 2016

Uh oh!

ahoppen commented May 5, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jrose-apple commented May 5, 2016

Uh oh!

jrose-apple commented May 5, 2016

Uh oh!

DougGregor commented May 6, 2016

Uh oh!

ahoppen commented May 7, 2016

Uh oh!

tkremenek commented May 8, 2016

Uh oh!

jrose-apple commented May 11, 2016

Uh oh!

bob-wilson commented May 11, 2016

Uh oh!

alexwlchan commented May 11, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bob-wilson commented May 11, 2016

Uh oh!

jrose-apple commented May 11, 2016

Uh oh!

tfiala commented May 11, 2016

Uh oh!

tkremenek commented May 11, 2016

Uh oh!

tkremenek commented May 11, 2016

Uh oh!

tkremenek commented May 11, 2016

Uh oh!

tkremenek commented May 11, 2016

Uh oh!

jrose-apple commented May 11, 2016

Uh oh!

slavapestov May 11, 2016

Choose a reason for hiding this comment

Uh oh!

lattner May 12, 2016

Choose a reason for hiding this comment

Uh oh!

ahoppen commented May 12, 2016

Uh oh!

tfiala commented May 12, 2016

Uh oh!

ahoppen commented May 21, 2016

ahoppen commented May 5, 2016 •

edited

Loading

alexwlchan commented May 11, 2016 •

edited

Loading