[QoI] Diagnose initializers written as typed patterns (SR-1461) #4371

jtbandes · 2016-08-18T08:22:14Z

What's in this pull request?

Diagnose and offer to replace var x: [Int]() with var x = [Int]().

error: unexpected initializer in pattern; did you mean to use '='?
var x: [Int]()
     ~ ~~~~~^~
      =

Resolved bug number: SR-1461

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
All supported platforms	@swift-ci Please smoke test and merge
OS X platform	@swift-ci Please smoke test OS X platform
Linux platform	@swift-ci Please smoke test Linux platform

A smoke test on macOS does the following:

Builds the compiler incrementally.
Builds the standard library only for macOS. Simulator standard libraries and
device standard libraries are not built.
lldb is not built.
The test and validation-test targets are run only for macOS. The optimized
version of these tests are not run.

A smoke test on Linux does the following:

Builds the compiler incrementally.
Builds the standard library incrementally.
lldb is built incrementally.
The swift test and validation-test targets are run. The optimized version of these
tests are not run.
lldb is tested.

Validation Testing

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

Lint Testing

Language	Comment
Python	@swift-ci Please Python lint

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

rintaro · 2016-08-18T11:23:52Z

lib/Parse/ParsePattern.cpp

+          }
+          diagnose(lParenLoc, diag::initializer_as_typed_pattern)
+          .highlightChars(Ty.get()->getStartLoc(),
+                          rParenLoc.getAdvancedLoc(1))


Why not .highlight({Ty.get()->getStartLoc(), rParenLoc})

Hmm, I guess I was wrong about the need to use getAdvancedLoc to highlight all the way to the closing paren. Will change.

slavapestov · 2016-08-19T03:25:01Z

lib/Parse/ParsePattern.cpp

+            replacement = "=";
+          }
+          diagnose(lParenLoc, diag::initializer_as_typed_pattern)
+          .highlight({Ty.get()->getStartLoc(), rParenLoc})


Indentation is wrong here

jrose-apple · 2016-08-19T21:13:08Z

lib/Parse/ParsePattern.cpp

+          diagnose(lParenLoc, diag::initializer_as_typed_pattern)
+            .highlight({Ty.get()->getStartLoc(), rParenLoc})
+            .fixItReplace(colonLoc, replacement);
+          result.setIsParseError();


I'm a little unhappy that the parsed expr doesn't end up anywhere. That will probably break syntax highlighting and code completion. Is it possible to put it into the AST somewhere?

Any ideas how? Might be able to hoist this into the caller, so if after parsing the pattern we find it's actually an initializer, we can parse it as such. It looked much easier the way it is when I implemented it, but I'll take another look later today.

Hm. @benlangmuir, @nkcsgexi, any ideas?

Hrm. I take it Swift's lexer isn't fancy enough to be able to modify the token stream? If we backtracked and replaced the : with `=' the caller would probably recover very well.

Short of that, yeah trying to handle more of this in the caller might be workable...

Can we at least preserve the TypeRepr, maybe parsed like var x: [Int]

oh, my bad. You do preserve the TypeRepr. For the expressions in call arg, I've no other suggestions except hoisting to the caller as another expression.

Hm, I guess there is a question of whether we want PatternBindingDecl parsing to generally recognize incorrect var x: «expr» and treat it as var x = «expr» instead.

(The fix as written is pretty simple because it only handles exprs that look like initializer calls. It handles var x: [Int](), but it won't handle var x: Unmanaged.fromOpaque(y) or var x: MemoryLayout<Int>.stride, for instance.)

In general, if the user made a typo and wrote : instead of =, the parser might only notice a problem after successfully parsing a typed pattern binding. So, unless we want to try parsing var x = «expr» first every time we look at a pattern (even if we see a :), I think this would need to be hoisted even further out than the caller of parseTypedPattern(), because it needs to invoke all the logic in parseExpr/parseExprPostfix, and it might need to do so even if parseTypedPattern succeeded.

Which is to say: the fix as written is pretty simple and self-contained. After reading some more of the parser implementation, I'm inclined to think producing better results than this would be considerably more involved. Feel free to prove me wrong, if you can think of a clean way to do it!

I like the idea of recognizing var x : <<expr>> as var x = <<expr>>. However, there may be other problems by doing so; for instance, if there is a function called getEqualPos() in the AST node, what will it return? an invalid position or the position of : instead.

The fixit per se is for sure an improvement! Thanks for working on that. Preserving erroneous AST nodes and providing insight of them is ideal for tooling but I have to say it is not easy in this case.

jrose-apple · 2016-08-19T21:15:01Z

This is a great fix to have.

jtbandes · 2016-08-20T07:13:27Z

@jrose-apple Thanks for the prompting on additional tests — that led me to discover trailing closures weren't handled properly at the top level, so I added a ContextChange. I also added a Backtrack for good measure (although if we find a ( and then can't parse an initializer call, there's not going to be anything we can successfully continue parsing anyway).

I also improved the logic for choosing a replacement string, so the fix-it won't produce invalid expressions like var x =[Int]().

@swift-ci please smoke test macOS

jrose-apple · 2016-08-22T15:21:03Z

lib/Parse/ParsePattern.cpp

+          // Suggest replacing ':' with '=' (ensuring proper whitespace).
+
+          bool needSpaceBefore =
+            SourceMgr.getByteDistance(pastEndOfPrevLoc, colonLoc) == 0;


This can just use ==.

jtbandes · 2016-08-22T16:00:42Z

@jrose-apple done!
@swift-ci smoke test

jtbandes · 2016-08-23T17:44:26Z

friendly bump @jrose-apple @DougGregor

jrose-apple · 2016-08-24T16:25:04Z

I'm happy if @nkcsgexi and @rintaro are happy with it.

DougGregor · 2016-08-24T16:31:25Z

LGTM!

rintaro · 2016-08-25T04:23:47Z

lib/Parse/ParsePattern.cpp

+
+          bool needSpaceBefore = (pastEndOfPrevLoc == colonLoc);
+          bool needSpaceAfter =
+            SourceMgr.getByteDistance(colonLoc, startOfNextLoc) <= 1;


@jtbandes
Sorry, this didn't work when a comment is involved:

let i:/* comment */Int(42) let i =/* comment */Int(42) // As of now, we can't parse this.

How do you think about #4499?

[pull] swiftwasm from main

rintaro reviewed Aug 18, 2016
View reviewed changes

jtbandes force-pushed the diag branch from f7b9872 to 23ee483 Compare August 18, 2016 15:14

slavapestov reviewed Aug 19, 2016
View reviewed changes

jtbandes force-pushed the diag branch from 23ee483 to cd6692b Compare August 19, 2016 07:26

jrose-apple reviewed Aug 19, 2016
View reviewed changes

jtbandes force-pushed the diag branch from cd6692b to 3c9bc92 Compare August 20, 2016 07:09

jrose-apple reviewed Aug 22, 2016
View reviewed changes

[QoI] diagnose initializers written as typed patterns (SR-1461)

73830df

jtbandes force-pushed the diag branch from 3c9bc92 to 73830df Compare August 22, 2016 16:00

DougGregor merged commit 9c943f0 into swiftlang:master Aug 24, 2016

jtbandes deleted the diag branch August 25, 2016 02:32

rintaro reviewed Aug 25, 2016
View reviewed changes

jtbandes mentioned this pull request Aug 26, 2016

Swift 3: pull in fixes from master branch #4516

Merged

jtbandes mentioned this pull request Aug 18, 2016

[SR-1461] bad error message when specifying type instead of value with empty Dictionary/Array initializers #44070

Closed

kateinoigakukun added a commit that referenced this pull request Aug 31, 2022

Merge pull request #4371 from swiftwasm/main

f3f8d8d

[pull] swiftwasm from main

[QoI] Diagnose initializers written as typed patterns (SR-1461) #4371

[QoI] Diagnose initializers written as typed patterns (SR-1461) #4371

Uh oh!

Conversation

jtbandes commented Aug 18, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What's in this pull request?

Resolved bug number: SR-1461

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!

jtbandes Aug 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

nkcsgexi Aug 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrose-apple commented Aug 19, 2016

Uh oh!

jtbandes commented Aug 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtbandes commented Aug 22, 2016

Uh oh!

jtbandes commented Aug 23, 2016

Uh oh!

jrose-apple commented Aug 24, 2016

Uh oh!

DougGregor commented Aug 24, 2016

Uh oh!

rintaro Aug 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jtbandes commented Aug 18, 2016 •

edited

Loading

jtbandes Aug 19, 2016 •

edited

Loading

nkcsgexi Aug 20, 2016 •

edited

Loading

jtbandes commented Aug 20, 2016 •

edited

Loading

rintaro Aug 25, 2016 •

edited

Loading