New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from Mar 29, 2016

Conversation

Projects
None yet
3 participants
@manavgabhawala
Contributor

manavgabhawala commented Mar 23, 2016

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.

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

@tkremenek

This comment has been minimized.

Show comment
Hide comment
@tkremenek

tkremenek Mar 24, 2016

Member

@swift-ci Please smoke test

Member

tkremenek commented Mar 24, 2016

@swift-ci Please smoke test

@tkremenek

This comment has been minimized.

Show comment
Hide comment
@tkremenek

tkremenek Mar 24, 2016

Member

@lattner can you review this change?

Member

tkremenek commented Mar 24, 2016

@lattner can you review this change?

@lattner

This comment has been minimized.

Show comment
Hide comment
@lattner

lattner Mar 24, 2016

Collaborator

Yes, I'll review it, hopefully tomorrow.

Collaborator

lattner commented Mar 24, 2016

Yes, I'll review it, hopefully tomorrow.

@manavgabhawala

This comment has been minimized.

Show comment
Hide comment
@manavgabhawala

manavgabhawala Mar 24, 2016

Contributor

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

Contributor

manavgabhawala commented Mar 24, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@tkremenek

tkremenek Mar 24, 2016

Member

@swift-ci please test

Member

tkremenek commented Mar 24, 2016

@swift-ci please test

Show outdated Hide outdated lib/Parse/ParsePattern.cpp
// ('inout' | 'let')?
bool hasSpecifier = false;
while (Tok.isAny(tok::kw_inout, tok::kw_let, tok::kw_var)) {

This comment has been minimized.

@lattner

lattner Mar 25, 2016

Collaborator

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

@lattner

lattner Mar 25, 2016

Collaborator

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

This comment has been minimized.

@manavgabhawala

manavgabhawala Mar 25, 2016

Contributor

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.

@manavgabhawala

manavgabhawala Mar 25, 2016

Contributor

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.

This comment has been minimized.

@lattner

lattner Mar 25, 2016

Collaborator

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

lattner Mar 25, 2016

Collaborator

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.

Show outdated Hide outdated lib/Parse/ParsePattern.cpp
if (Tok.isAny(tok::kw_inout, tok::kw_let, tok::kw_var)) {
diagnose(Tok, diag::parameter_inout_var_let)
consumeToken();
param.isInvalid = true;

This comment has been minimized.

@lattner

lattner Mar 25, 2016

Collaborator

In terms of migration, it seems best for the parser to recognize this as a valid parameter, and then have Sema see it and suggest a rewrite, which inserts a local shadow into the body of the function, rather than removing it with no migration.

@lattner

lattner Mar 25, 2016

Collaborator

In terms of migration, it seems best for the parser to recognize this as a valid parameter, and then have Sema see it and suggest a rewrite, which inserts a local shadow into the body of the function, rather than removing it with no migration.

Show outdated Hide outdated test/FixCode/fixits-apply.swift.result
@@ -33,7 +33,7 @@ func supported() -> MyMask {
func foo() -> Int {
do {
} catch let err {

This comment has been minimized.

@lattner

lattner Mar 25, 2016

Collaborator

I don't understand why you made this test suite change, it doesn't seem necessary. If it is necessary, then there is something else going wrong. We want to still allow "catch let err".

@lattner

lattner Mar 25, 2016

Collaborator

I don't understand why you made this test suite change, it doesn't seem necessary. If it is necessary, then there is something else going wrong. We want to still allow "catch let err".

@lattner

This comment has been minimized.

Show comment
Hide comment
@lattner

lattner Mar 25, 2016

Collaborator

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

Collaborator

lattner commented Mar 25, 2016

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

@manavgabhawala

This comment has been minimized.

Show comment
Hide comment
@manavgabhawala

manavgabhawala Mar 26, 2016

Contributor

@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.

Contributor

manavgabhawala commented Mar 26, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@lattner

lattner Mar 28, 2016

Collaborator

@swift-ci please test

Collaborator

lattner commented Mar 28, 2016

@swift-ci please test

Show outdated Hide outdated 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", ())

This comment has been minimized.

@lattner

lattner Mar 28, 2016

Collaborator

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

lattner Mar 28, 2016

Collaborator

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.

Show outdated Hide outdated lib/Parse/ParsePattern.cpp
param.isInvalid = true;
} else if (Tok.is(tok::kw_let)) {
diagnose(Tok.getLoc(), diag::parameter_let_as_attr).
fixItRemove(Tok.getLoc());

This comment has been minimized.

@lattner

lattner Mar 28, 2016

Collaborator

I don't think that you need to catch the case of let/var after the colon, particularly since these are going to be removed from swift parameters in general. However, if you really want to handle this, please merging the two parameter_var_as_attr/parameter_let_as_attr diagnostics into a single one that takes a bool (to tell it whether to print let or var) which it can dispatch on with %select(let|var)0 in the diagnostic text.

@lattner

lattner Mar 28, 2016

Collaborator

I don't think that you need to catch the case of let/var after the colon, particularly since these are going to be removed from swift parameters in general. However, if you really want to handle this, please merging the two parameter_var_as_attr/parameter_let_as_attr diagnostics into a single one that takes a bool (to tell it whether to print let or var) which it can dispatch on with %select(let|var)0 in the diagnostic text.

This comment has been minimized.

@manavgabhawala

manavgabhawala Mar 28, 2016

Contributor

Do you think SE-0053 is highly likely to be accepted? If so it would be almost trivial to implement and merge in with this PR and also fix the diagnostics to be more uniform since both var and let will be disallowed.

@manavgabhawala

manavgabhawala Mar 28, 2016

Contributor

Do you think SE-0053 is highly likely to be accepted? If so it would be almost trivial to implement and merge in with this PR and also fix the diagnostics to be more uniform since both var and let will be disallowed.

This comment has been minimized.

@lattner

lattner Mar 28, 2016

Collaborator

Yes, it is a virtual certainty that SE-0053 will be accepted. Agreed, go for it!

@lattner

lattner Mar 28, 2016

Collaborator

Yes, it is a virtual certainty that SE-0053 will be accepted. Agreed, go for it!

This comment has been minimized.

@manavgabhawala

manavgabhawala Mar 28, 2016

Contributor

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

@manavgabhawala

manavgabhawala Mar 28, 2016

Contributor

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

Show outdated Hide outdated lib/Sema/TypeCheckPattern.cpp
} else {
// Insert the shadow copy. The computations that follow attempt to
// 'best guess' the indentation and new lines so that the user
// doesn't have to add any whitespace

This comment has been minimized.

@lattner

lattner Mar 28, 2016

Collaborator

Totally pedantic, but please end comment sentences with a period.

@lattner

lattner Mar 28, 2016

Collaborator

Totally pedantic, but please end comment sentences with a period.

Show outdated Hide outdated lib/Sema/TypeCheckPattern.cpp
auto startingString = CharSourceRange(TC.Context.SourceMgr,
declBody->getLBraceLoc(),
insertionStartLoc).str().str();
bool sawNewline = false;

This comment has been minimized.

@lattner

lattner Mar 28, 2016

Collaborator

I'm not sure what this is doing. Why not just ask the SourceMgr for the column location of the location in question?

@lattner

lattner Mar 28, 2016

Collaborator

I'm not sure what this is doing. Why not just ask the SourceMgr for the column location of the location in question?

This comment has been minimized.

@manavgabhawala

manavgabhawala Mar 28, 2016

Contributor

I'm really new to compilers so I didn't realize that there was a column location on SourceMgr. But, in any case how else could we determine whether to insert spaces or tabs for the indentation with just the column location info?

@manavgabhawala

manavgabhawala Mar 28, 2016

Contributor

I'm really new to compilers so I didn't realize that there was a column location on SourceMgr. But, in any case how else could we determine whether to insert spaces or tabs for the indentation with just the column location info?

This comment has been minimized.

@lattner

lattner Mar 28, 2016

Collaborator

Ah, fair point. What is other similar code doing for this? Can you please factor that code out into something shared (like "getLeadingWhitespace(...)", maybe put it as a helper on SourceManager?

@lattner

lattner Mar 28, 2016

Collaborator

Ah, fair point. What is other similar code doing for this? Can you please factor that code out into something shared (like "getLeadingWhitespace(...)", maybe put it as a helper on SourceManager?

Show outdated Hide outdated lib/Sema/TypeCheckPattern.cpp
.fixItRemove(decl->getLetVarInOutLoc())
.fixItInsertAfter(declBody->getLBraceLoc(),
startingString + "var " + parameterName + " = " +
parameterName + ending);

This comment has been minimized.

@lattner

lattner Mar 28, 2016

Collaborator

So awesome!

@lattner

lattner Mar 28, 2016

Collaborator

So awesome!

@lattner

This comment has been minimized.

Show comment
Hide comment
@lattner

lattner Mar 28, 2016

Collaborator

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

Collaborator

lattner commented Mar 28, 2016

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

@lattner

This comment has been minimized.

Show comment
Hide comment
@lattner

lattner Mar 28, 2016

Collaborator

To confirm, SE-0053 is accepted.

Collaborator

lattner commented Mar 28, 2016

To confirm, SE-0053 is accepted.

@manavgabhawala

This comment has been minimized.

Show comment
Hide comment
@manavgabhawala

manavgabhawala Mar 29, 2016

Contributor

@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.

Contributor

manavgabhawala commented Mar 29, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@lattner

lattner Mar 29, 2016

Collaborator

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.

Collaborator

lattner commented Mar 29, 2016

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

This comment has been minimized.

Show comment
Hide comment
@lattner

lattner Mar 29, 2016

Collaborator

@swift-ci please test

Collaborator

lattner commented Mar 29, 2016

@swift-ci please test

@lattner

This comment has been minimized.

Show comment
Hide comment
@lattner

lattner Mar 29, 2016

Collaborator

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

Collaborator

lattner commented Mar 29, 2016

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

@manavgabhawala

This comment has been minimized.

Show comment
Hide comment
@manavgabhawala

manavgabhawala Mar 29, 2016

Contributor

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

Contributor

manavgabhawala commented Mar 29, 2016

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

@lattner

This comment has been minimized.

Show comment
Hide comment
@lattner

lattner Mar 29, 2016

Collaborator

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

Collaborator

lattner commented Mar 29, 2016

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

@manavgabhawala

This comment has been minimized.

Show comment
Hide comment
@manavgabhawala

manavgabhawala Mar 29, 2016

Contributor

Since it still crashes, I just adjusted the inout position so that the bug causing the crash is still tracked and left it in the crashes folder with the --crash line.

Contributor

manavgabhawala commented Mar 29, 2016

Since it still crashes, I just adjusted the inout position so that the bug causing the crash is still tracked and left it in the crashes folder with the --crash line.

@lattner

This comment has been minimized.

Show comment
Hide comment
@lattner

lattner Mar 29, 2016

Collaborator

Ok, reviewing the patch now.

Collaborator

lattner commented Mar 29, 2016

Ok, reviewing the patch now.

Show outdated Hide outdated 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))

This comment has been minimized.

@lattner

lattner Mar 29, 2016

Collaborator

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()"

@lattner

lattner Mar 29, 2016

Collaborator

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()"

This comment has been minimized.

@lattner

lattner Mar 29, 2016

Collaborator

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

@lattner

lattner Mar 29, 2016

Collaborator

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

Show outdated Hide outdated lib/Parse/ParsePattern.cpp
param.isInvalid = true;
} else {
hasSpecifier = true;
param.LetVarInOutLoc = consumeToken();

This comment has been minimized.

@lattner

lattner Mar 29, 2016

Collaborator

nit-picky, but both of these can use "... = consumeToken(tok::kw_inout);" since we you the specific token you expect. This just enables an assertion in the parser, it isn't a functionality change.

@lattner

lattner Mar 29, 2016

Collaborator

nit-picky, but both of these can use "... = consumeToken(tok::kw_inout);" since we you the specific token you expect. This just enables an assertion in the parser, it isn't a functionality change.

Show outdated Hide outdated lib/Parse/ParsePattern.cpp
while (Tok.is(tok::kw_inout)) {
hasValidInOut = true;
if (hasSpecifier) {
diagnose(Tok.getLoc() , diag::parameter_inout_var_let_repeated)

This comment has been minimized.

@lattner

lattner Mar 29, 2016

Collaborator

Don't need a space before the comma.

@lattner

lattner Mar 29, 2016

Collaborator

Don't need a space before the comma.

Show outdated Hide outdated lib/Sema/TypeCheckPattern.cpp
if (!innermostMethodCtx || !innermostMethodCtx->hasBody()) {
// If there is not function body, just suggest removal of 'var'
TC.diagnose(decl->getLetVarInOutLoc(), diag::var_parameter_not_allowed)
.fixItRemove(decl->getLetVarInOutLoc());

This comment has been minimized.

@lattner

lattner Mar 29, 2016

Collaborator

Is this really enough to fix SR-1020? It seems like it would leave the space. Maybe we need a ".fixItRemoveWithTrailingWhitespace" operation. I'm sure that this comes up somewhere else in the compiler as well (grep for .fixItRemove and look for some code doing CharSourceRange gymnastics).

@lattner

lattner Mar 29, 2016

Collaborator

Is this really enough to fix SR-1020? It seems like it would leave the space. Maybe we need a ".fixItRemoveWithTrailingWhitespace" operation. I'm sure that this comes up somewhere else in the compiler as well (grep for .fixItRemove and look for some code doing CharSourceRange gymnastics).

This comment has been minimized.

@manavgabhawala

manavgabhawala Mar 29, 2016

Contributor

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

// If we're removing something (e.g. a keyword), do a bit of extra work to
// make sure that we leave the code in a good place, without extraneous white
// space around its hole. Specifically, check to see there is whitespace
// before and after the end of range. If so, nuke the space afterward to keep
// things consistent.

And the test case FixIt/fixits-apply.swift check to make sure that after the fix it there is no surrounding space with the var. So, unless I'm missing something this should be fine?

@manavgabhawala

manavgabhawala Mar 29, 2016

Contributor

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

// If we're removing something (e.g. a keyword), do a bit of extra work to
// make sure that we leave the code in a good place, without extraneous white
// space around its hole. Specifically, check to see there is whitespace
// before and after the end of range. If so, nuke the space afterward to keep
// things consistent.

And the test case FixIt/fixits-apply.swift check to make sure that after the fix it there is no surrounding space with the var. So, unless I'm missing something this should be fine?

Show outdated Hide outdated lib/Sema/TypeCheckPattern.cpp
if (!declBody->getNumElements()) {
// emtpy function body
auto rBraceLine = SM.getLineNumber(declBody->getRBraceLoc());

This comment has been minimized.

@lattner

lattner Mar 29, 2016

Collaborator

I'd suggest hoisting this call out of the if since both arms need it. Doing so would increase readability of the code by making the left/right side more analogous.

@lattner

lattner Mar 29, 2016

Collaborator

I'd suggest hoisting this call out of the if since both arms need it. Doing so would increase readability of the code by making the left/right side more analogous.

// If the param is not a 'let' and it is not an 'inout'.
// It must be a 'var'. Provide helpful diagnostics like a shadow copy
// in the function body to fix the 'var' attribute.
if (!decl->isLet() && !Ty->is<InOutType>()) {

This comment has been minimized.

@lattner

lattner Mar 29, 2016

Collaborator

This whole blob of code is pretty detailed and separable for the rest of the logic in validateParameterType. Please split the body of the if out to a new helper function, so we get something more like:

  • // If the param is not a 'let' and it is not an 'inout'.
    // It must be a 'var'. Provide helpful diagnostics like a shadow copy
    // in the function body to fix the 'var' attribute.
    if (!decl->isLet() && !Ty->is())
    diagnoseAndMigrateVarParameter(...)

or whatever.

@lattner

lattner Mar 29, 2016

Collaborator

This whole blob of code is pretty detailed and separable for the rest of the logic in validateParameterType. Please split the body of the if out to a new helper function, so we get something more like:

  • // If the param is not a 'let' and it is not an 'inout'.
    // It must be a 'var'. Provide helpful diagnostics like a shadow copy
    // in the function body to fix the 'var' attribute.
    if (!decl->isLet() && !Ty->is())
    diagnoseAndMigrateVarParameter(...)

or whatever.

Show outdated Hide outdated lib/Sema/TypeCheckPattern.cpp
auto lBraceLine = SM.getLineNumber(declBody->getLBraceLoc());
if (!declBody->getNumElements()) {
// emtpy function body

This comment has been minimized.

@lattner

lattner Mar 29, 2016

Collaborator

Typo "empty", haha got to it before @practicalswift did :-)

@lattner

lattner Mar 29, 2016

Collaborator

Typo "empty", haha got to it before @practicalswift did :-)

Show outdated Hide outdated lib/Sema/TypeCheckPattern.cpp
if (lBraceLine == rBraceLine) {
// Same line braces, means we probably have something
// like {} as the func body. Insert directly into body

This comment has been minimized.

@lattner

lattner Mar 29, 2016

Collaborator

Please end all comments like this with a period, this occurs in several places.

@lattner

lattner Mar 29, 2016

Collaborator

Please end all comments like this with a period, this occurs in several places.

@lattner

This comment has been minimized.

Show comment
Hide comment
@lattner

lattner Mar 29, 2016

Collaborator

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!

Collaborator

lattner commented Mar 29, 2016

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

This comment has been minimized.

Show comment
Hide comment
@manavgabhawala

manavgabhawala Mar 29, 2016

Contributor

@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 :)

Contributor

manavgabhawala commented Mar 29, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@lattner

lattner Mar 29, 2016

Collaborator

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!

Collaborator

lattner commented Mar 29, 2016

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

This comment has been minimized.

Show comment
Hide comment
@lattner

lattner Mar 29, 2016

Collaborator

@swift-ci Please test

Collaborator

lattner commented Mar 29, 2016

@swift-ci Please test

@lattner

This comment has been minimized.

Show comment
Hide comment
@lattner

lattner Mar 29, 2016

Collaborator

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!

Collaborator

lattner commented Mar 29, 2016

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

This comment has been minimized.

Show comment
Hide comment
@lattner

lattner Mar 29, 2016

Collaborator

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

Collaborator

lattner commented Mar 29, 2016

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

[Parser] Cleans up parsing of parameter attributes. Implements SE-005…
…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

This comment has been minimized.

Show comment
Hide comment
@manavgabhawala

manavgabhawala Mar 29, 2016

Contributor

@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?

Contributor

manavgabhawala commented Mar 29, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@lattner

lattner Mar 29, 2016

Collaborator

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

Collaborator

lattner commented Mar 29, 2016

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

@lattner

This comment has been minimized.

Show comment
Hide comment
@lattner

lattner Mar 29, 2016

Collaborator

@swift-ci please test

Collaborator

lattner commented Mar 29, 2016

@swift-ci please test

@lattner

This comment has been minimized.

Show comment
Hide comment
@lattner

lattner Mar 29, 2016

Collaborator

@swift-ci Please test

Collaborator

lattner commented Mar 29, 2016

@swift-ci Please test

@manavgabhawala

This comment has been minimized.

Show comment
Hide comment
@manavgabhawala

manavgabhawala Mar 29, 2016

Contributor

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

Contributor

manavgabhawala commented Mar 29, 2016

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

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

1 of 2 checks passed

Swift Test Linux Platform Build finished. No test results found.
Details
Swift Test OS X Platform Build finished. 32428 tests run, 0 skipped, 0 failed.
Details
@lattner

This comment has been minimized.

Show comment
Hide comment
@lattner

lattner Mar 29, 2016

Collaborator

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

Collaborator

lattner commented Mar 29, 2016

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment