[Parser] Cleans up parsing of function parameter attributes #1812

manavgabhawala · 2016-03-23T21:56:58Z

What's in this pull request?

This pull request cleans up parsing of parameter attributes by parsing the inout, let and var tokens better. It cleans up the implementation to work better with SE-0003 by giving better fix-its and diagnostics for var as parameter attributes. It implements SE-0053 to disallow let as an attribute. It provides better fixits for inout that are either duplicated or misplaced to better implement SE-0031.

Resolved bug number:

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.

tkremenek · 2016-03-24T05:18:27Z

@swift-ci Please smoke test

tkremenek · 2016-03-24T05:19:47Z

@lattner can you review this change?

lattner · 2016-03-24T05:50:01Z

Yes, I'll review it, hopefully tomorrow.

manavgabhawala · 2016-03-24T19:34:24Z

@lattner or @tkremenek can you please run the tests again? I fixed all the ones that were failing and squashed to a single commit.

tkremenek · 2016-03-24T23:08:42Z

@swift-ci please test

lattner · 2016-03-25T05:33:53Z

lib/Parse/ParsePattern.cpp

+
+    // ('inout' | 'let')?
+    bool hasSpecifier = false;
+    while (Tok.isAny(tok::kw_inout, tok::kw_let, tok::kw_var)) {


I'm not sure why you changed the comment here, given that kw_var is still accepted.

We check for kw_var to give the warning but the code would be invalid if var was still there. I'm not sure whether the comment is supposed to reflect what's valid or what's checked for.

I'd prefer to have the in-function comments correlate to the code. It makes sense to keep the comments at the top of the function aligned with the formal grammar.

lattner · 2016-03-25T05:37:00Z

I made a few comments above, the test failures seem spurious.

manavgabhawala · 2016-03-26T05:33:40Z

@lattner I changed it so that the 'var' detection now happens in sema, and provides a nifty fixit to place a shadow copy. I tried to make it so that indentation would work too, but tbh I'm not completely sure that's the right way. I also fixed the comments, and the test case with the let/var diff, I moved it to another test case because the diagnostic was not getting a chance to run because of all the other errors in the file.

lattner · 2016-03-28T03:40:08Z

@swift-ci please test

lattner · 2016-03-28T03:42:24Z

include/swift/AST/DiagnosticsParse.def

+ERROR(parameter_let_as_attr,none,
+	  "'let' before a parameter type is not allowed, place it before the parameter name instead", ())
+ERROR(parameter_var_as_attr,none,
+"'var' as a parameter attribute is not allowed, consider creating a shadow copy in the function body or using 'inout' for pass-by-reference", ())


Please indent the "'var... line to match the placement of the "(parameter_var_as_attr" on the previous line. Similarly, please de-indent the "'let' before ... " line a few lines above.

lattner · 2016-03-28T03:48:46Z

Detailed comments in the patch. This is looking great, thank you for working on this!

lattner · 2016-03-28T20:05:16Z

To confirm, SE-0053 is accepted.

manavgabhawala · 2016-03-29T04:23:31Z

@lattner SE-0053 is implemented along with all the other changes you requested. Turns out the Lexer already had a function for determining whitespace so I used that instead of making a new one in the SourceManager.

lattner · 2016-03-29T04:37:53Z

Okay great. Should we then allow let var and inout be argument labels, or warn/error and give fixit information for those?

I think we should warn/error on them with migration away from the syntax where possible. That said, we shouldn't bother with this if they occur after the colon, since that is not syntax we have ever accepted.

lattner · 2016-03-29T04:37:59Z

@swift-ci please test

lattner · 2016-03-29T05:02:05Z

The test failure is a win: you fixed a crasher :-)

manavgabhawala · 2016-03-29T05:07:41Z

Unfortunately, it still crashes with the inout moved to the right location.

lattner · 2016-03-29T05:12:47Z

Yep, please, and remove the --crash from the run line. Thanks!

lattner · 2016-03-29T05:35:16Z

Ok, reviewing the patch now.

lattner · 2016-03-29T05:36:52Z

include/swift/AST/DiagnosticsParse.def

      "parameter may not have multiple 'inout', 'var', or 'let' specifiers",
      ())
+ERROR(parameter_let_as_attr,none,
+      "'let' as a parameter attribute is not allowed", (bool))



The (bool) parameter is not used and is always false. You can drop it. I was thinking that you can do something like:

"'%select{let|var}0' as a parameter attribute is not allowed"

Where the bool is "isVar()"

But given your logic, I can see how that doesn't make sense anymore! So I think you should just drop the bool.

lattner · 2016-03-29T05:50:03Z

And... that's all I've got, some of them are super picky :-) Thank you again for working on this, the fixits & migration that you're providing are really top notch!

manavgabhawala · 2016-03-29T06:49:43Z

@lattner fixed everything, I think. Thank you so much for your peer reviews, its been really awesome to have the legendary Chris Lattner review my code :)

lattner · 2016-03-29T16:38:02Z

If I'm not mistaken, fixItRemove already handles this. In fact the comments there talk about this issue to (quoted below):

Ah, you're right, thanks!

lattner · 2016-03-29T16:38:12Z

@swift-ci Please test

lattner · 2016-03-29T16:43:06Z

The revised patch looks great. Once testing is done, I'll merge it.

In a follow-on PR, please update the changelog to mention the changes to formerly valid code (e.g. that inout/var/let before the colon) are now errors. Also, please add some a test to show that the invalid code is being rejected with the new diagnostics. Thanks again!

lattner · 2016-03-29T16:55:59Z

It looks like your refactor introduced some crashes:

swift: /home/buildnode/jenkins/workspace/swift-PR-Linux/swift/lib/AST/DiagnosticEngine.cpp:188: swift::InFlightDiagnostic &swift::InFlightDiagnostic::fixItReplaceChars(swift::SourceLoc, swift::SourceLoc, llvm::StringRef): Assertion `IsActive && "Cannot modify an inactive diagnostic"' failed.
#0 0x0000000003198f98 llvm::sys::PrintStackTrace(llvm::raw_ostream&) (/home/buildnode/jenkins/workspace/swift-PR-Linux/buildbot_linux/swift-linux-x86_64/bin/swift+0x3198f98)
#1 0x0000000003197766 llvm::sys::RunSignalHandlers() (/home/buildnode/jenkins/workspace/swift-PR-Linux/buildbot_linux/swift-linux-x86_64/bin/swift+0x3197766)
#2 0x0000000003199aca SignalHandler(int) (/home/buildnode/jenkins/workspace/swift-PR-Linux/buildbot_linux/swift-linux-x86_64/bin/swift+0x3199aca)
#3 0x00007f55cb21e340 restore_rt (/lib/x86_64-linux-gnu/libpthread.so.0+0x10340)
#4 0x00007f55c9bf5cc9 gsignal (/lib/x86_64-linux-gnu/libc.so.6+0x36cc9)
#5 0x00007f55c9bf90d8 abort (/lib/x86_64-linux-gnu/libc.so.6+0x3a0d8)
#6 0x00007f55c9beeb86 (/lib/x86_64-linux-gnu/libc.so.6+0x2fb86)
#7 0x00007f55c9beec32 (/lib/x86_64-linux-gnu/libc.so.6+0x2fc32)
#8 0x000000000108a560 swift::InFlightDiagnostic::fixItReplaceChars(swift::SourceLoc, swift::SourceLoc, llvm::StringRef) (/home/buildnode/jenkins/workspace/swift-PR-Linux/buildbot_linux/swift-linux-x86_64/bin/swift+0x108a560)
#9 0x0000000000eb9285 validateParameterType(swift::ParamDecl, swift::DeclContext, swift::OptionSet<swift::TypeResolutionFlags, unsigned int>, swift::GenericTypeResolver_, swift::TypeChecker&) (/home/buildnode/jenkins/workspace/swift-PR-Linux/buildbot_linux/swift-linux-x86_64/bin/swift+0xeb9285)
#10 0x0000000000eb8c05 swift::TypeChecker::typeCheckParameterList(swift::ParameterList_, swift::DeclContext_, swift::OptionSet<swift::TypeResolutionFlags, unsigned int>, swift::GenericTypeResolver_) (/home/buildnode/jenkins/workspace/swift-PR-Linux/buildbot_linux/swift-linux-x86_64/bin/swift+0xeb8c05)
#11 0x0000000000e8f3d4 (anonymous namespace)::DeclChecker::visitFuncDecl(swift::FuncDecl*) (/home/buildnode/jenkins/workspace/swift-PR-Linux/buildbot_linux/swift-linux-x86_64/bin/swift+0xe8f3d4)
#12

…3. Fixes SR-979, SR-1020 and cleans up implementation of SE-0003. Provides better fix-its and diagnostics for misplaced 'inout' and prohibits 'var' and 'let' from parameter attributes

manavgabhawala · 2016-03-29T17:58:45Z

@lattner Should be fixed now. For the follow up PR do you want me to add tests to a separate file because this one already modifies and adds some tests that check for invalid var, let and inout. Specifically, Parse/invalid.swift and Sema/immutablility.swift run these tests and expect errors. What kind of tests you think I should add in addition to these, or are these enough?

lattner · 2016-03-29T17:59:21Z

Ah, if these are already covered, then we're good, thanks!

lattner · 2016-03-29T17:59:27Z

@swift-ci please test

lattner · 2016-03-29T17:59:47Z

@swift-ci Please test

manavgabhawala · 2016-03-29T19:30:49Z

@lattner it looks like the linux build timed out. Is there anything I need to fix or will running it again fix it?

lattner · 2016-03-29T20:23:08Z

It should be fine, I'll run some local tests. Thanks again for implementing this! Please close out the SR's, and send a PR for the changelog.

-Chris

Pull from the `main` upstream branch

manavgabhawala changed the title ~~Cleans up parsing of function parameter attributes~~ [Parser] Cleans up parsing of function parameter attributes Mar 23, 2016

tkremenek assigned lattner Mar 24, 2016

manavgabhawala force-pushed the master branch from 654a427 to 3c14577 Compare March 24, 2016 09:47

lattner reviewed Mar 25, 2016
View reviewed changes

manavgabhawala force-pushed the master branch from 3c14577 to cea3104 Compare March 26, 2016 05:30

lattner reviewed Mar 28, 2016
View reviewed changes

manavgabhawala force-pushed the master branch from cea3104 to a24eb3d Compare March 29, 2016 04:20

manavgabhawala force-pushed the master branch from a24eb3d to 39bd4d4 Compare March 29, 2016 05:06

lattner reviewed Mar 29, 2016
View reviewed changes

manavgabhawala force-pushed the master branch 2 times, most recently from e69267c to fc3b63c Compare March 29, 2016 06:46

[Parser] Cleans up parsing of parameter attributes. Implements SE-005…

7862f10

…3. Fixes SR-979, SR-1020 and cleans up implementation of SE-0003. Provides better fix-its and diagnostics for misplaced 'inout' and prohibits 'var' and 'let' from parameter attributes

manavgabhawala force-pushed the master branch from fc3b63c to 7862f10 Compare March 29, 2016 17:56

lattner merged commit 34ca3e9 into swiftlang:master Mar 29, 2016

manavgabhawala mentioned this pull request Mar 29, 2016

[CHANGELOG][Docs] Update CHANGELOG and Docs to reflect new parsing of function parameter attributes like 'var', 'let' and 'inout' for SE-0003, SE-0031 and SE-0053 #1930

Merged

1 task

MaxDesiatov added a commit that referenced this pull request Oct 19, 2020

Merge pull request #1812 from swiftwasm/maxd/pull-main

60b22ce

Pull from the `main` upstream branch

This was referenced Mar 23, 2016

[SR-979] Two inout crash compiler and source editor (Swift DEV Snapshots 2016-03-16) #43591

Closed

[SR-1020] Fix-it Delete “var” doesn't delete space before parameter name #43632

Closed

[Parser] Cleans up parsing of function parameter attributes #1812

[Parser] Cleans up parsing of function parameter attributes #1812

Conversation

manavgabhawala commented Mar 23, 2016

What's in this pull request?

Resolved bug number:

tkremenek commented Mar 24, 2016

tkremenek commented Mar 24, 2016

lattner commented Mar 24, 2016

manavgabhawala commented Mar 24, 2016

tkremenek commented Mar 24, 2016

lattner Mar 25, 2016

Choose a reason for hiding this comment

manavgabhawala Mar 25, 2016

Choose a reason for hiding this comment

lattner Mar 25, 2016

Choose a reason for hiding this comment

lattner commented Mar 25, 2016

manavgabhawala commented Mar 26, 2016

lattner commented Mar 28, 2016

lattner Mar 28, 2016

Choose a reason for hiding this comment

lattner commented Mar 28, 2016

lattner commented Mar 28, 2016

manavgabhawala commented Mar 29, 2016

lattner commented Mar 29, 2016

lattner commented Mar 29, 2016

lattner commented Mar 29, 2016

manavgabhawala commented Mar 29, 2016

lattner commented Mar 29, 2016

lattner commented Mar 29, 2016

lattner Mar 29, 2016

Choose a reason for hiding this comment

lattner Mar 29, 2016

Choose a reason for hiding this comment

lattner commented Mar 29, 2016

manavgabhawala commented Mar 29, 2016

lattner commented Mar 29, 2016

lattner commented Mar 29, 2016

lattner commented Mar 29, 2016

lattner commented Mar 29, 2016

manavgabhawala commented Mar 29, 2016

lattner commented Mar 29, 2016

lattner commented Mar 29, 2016

lattner commented Mar 29, 2016

manavgabhawala commented Mar 29, 2016

lattner commented Mar 29, 2016