[Parser] Fixes for ASTScope lookup #26095

davidungar · 2019-07-11T18:51:00Z

Track real EndLoc of TypeAliasDecl and SubscriptDecl.
Ensure TypeAliasDecl gets added to AST when doing completion.

davidungar · 2019-07-11T18:51:41Z

@harlanhaskins I had to change two of your tests. Is that OK?

davidungar · 2019-07-11T18:52:04Z

@swift-ci please smoke test

davidungar · 2019-07-11T18:54:10Z

Since the parser interacts so much with a lot of things, I checked in with @akyrtzi and he suggested I run these changes by you. Thanks!

harlanhaskins · 2019-07-11T18:58:06Z

test/SourceKit/CodeFormat/indent-implicit-getter.swift

@@ -63,7 +63,7 @@ class var foo: Int {
 // CHECK: key.sourcetext: "    subscript(a: Int) -> Int {"
 // CHECK: key.sourcetext: "        let x = 3"
 // CHECK: key.sourcetext: "        return x"
-// CHECK: key.sourcetext: "    }"
+// CHECK: key.sourcetext: "        }"


Hm, this looks a little suspicious. Should this have added indentation, @rintaro?

\cc @nkcsgexi, this is a regression for indentation, please don't change the test.

Thank you both. I found my mistake, fixed it, and changed the tests back. Hope it's good now.

davidungar · 2019-07-11T19:54:11Z

Thanks, guys, for looking at this so quickly! I'll look into why I thought I needed to change the test as soon as I can build out the bits here.

davidungar · 2019-07-11T22:14:14Z

@swift-ci please smoke test os x platform

include/swift/AST/Decl.h

lib/AST/Decl.cpp

benlangmuir · 2019-07-11T23:12:14Z

lib/Parse/ParseDecl.cpp

@@ -6623,6 +6626,8 @@ Parser::parseDeclSubscript(SourceLoc StaticLoc,
                   Flags, StaticLoc, Attributes,
                   ElementTy.get(), Indices.get(), Decls);

+  Subscript->setEndLoc(getEndOfPreviousLoc().getAdvancedLoc(-1));


I'm not thrilled with setting the end loc here, because if we ever add more early returns it's easy to forget it needs this as well. Is there something we can do to ensure it's always set?

I like that worry. I'll think on it.

OK, I'm trying to changes (not pushed yet):

Don't need to set the end loc before the first return, which does not return the SubscriptDecl.

Set the end loc as soon as the parsing is finished, moving that line up. And adding a comment.

I'll see how this works.

jrose-apple

The end location is the location of the last token, not the location of the last character in the last token.

davidungar · 2019-07-12T01:37:22Z

Thanks @jrose-apple I'll have to look further at that.

davidungar · 2019-07-12T04:09:15Z

@swift-ci please smoke test os x platform

davidungar · 2019-07-12T16:44:02Z

@benlangmuir @akyrtzi @jrose-apple Great suggestions! I think I've addressed them. Would you like to take another look when convenient? TIA.

davidungar · 2019-07-12T17:29:58Z

Thank you @akyrtzi and @benlangmuir for getting back to this so quickly!

davidungar · 2019-07-12T17:31:03Z

@swift-ci please smoke test linux platform

jrose-apple · 2019-07-12T18:33:30Z

include/swift/AST/Decl.h

@@ -5471,6 +5476,7 @@ enum class ObjCSubscriptKind {
 class SubscriptDecl : public GenericContext, public AbstractStorageDecl {
  SourceLoc StaticLoc;
  SourceLoc ArrowLoc;
+  SourceLoc EndLoc;


Do properties have the same problem? After all, they can also have unparsed bodies. See PatternBindingDecl::getSourceRange.

If that's the case, this should probably go into AbstractStorageDecl::AccessorRecord instead. You could even share the storage currently used for SourceRange Braces by changing it to something like SourceLoc BraceStartLoc; /* which may be invalid */ and SourceLoc EndLoc; /* which is always valid */.

Thanks for the thoughts. I am tempted by the idea of moving it up to AbstractStorageDecls, although it's not needed right now. I'm not crazy about reusing the storage in AccessorRecord because the concept of the end of an AbstractStorageDecl seems unrelated to me. It feels like it creates brittleness to mix it in there, and the need for validity comments has a bad smell. Given that, I don't favor moving the end loc up the class hierarchy because, being unused for VarDecls, nothing would break if the computation were wrong.
And there's the storage penalty in that case.

So, after thinking about it, I favor leaving the EndLoc where it is. What do you think?

I'm not sure what you mean about it being unused for VarDecls. Looking at the code, I don't see how a PatternBindingDecl would have the right source range for a variable with an unparsed inline getter.

I'm coming back to this now that ASTScopes seem to work for building Swift.o. @slavapestov suggested landing it with the default turned on just for Swift.o, and I like the idea. But I'll need to land a few small PRs first, including this one. Would you be OK with not pushing this up for now? I'd rather work on landing the ASTScope stuff, and it doesn't seem to need a SourceRange change for VarDecls. I can file a radar to go back and look at it later if you like.

All right, that sounds fair!

Thanks! I've created the radar. Will await any feedback on the latest two commits.

davidungar · 2019-07-25T04:46:01Z

@swift-ci please smoke test

davidungar · 2019-07-25T04:56:56Z

@akyrtzi @benlangmuir @jrose-apple I found two more changes for ASTScopes: First, when delayed-parsing a Decl in a closure body, the Decl needs to be added for the body. That's the subject of the penultimate commit. Second, ensure that an accessor can be found from its VarDecl, even in when invalid. Would you like to take a look at these? Thanks!

davidungar · 2019-07-25T05:31:31Z

@swift-ci please smoke test

1. Track real EndLoc of TypeAliasDecl and SubscriptDecl. 2. Ensure TypeAliasDecl gets added to AST when doing completion.

davidungar · 2019-07-25T15:45:06Z

@swift-ci please smoke test os x platform

jrose-apple · 2019-07-25T16:50:29Z

lib/Parse/ParseDecl.cpp

+        auto *body = CE->getBody();
+        SmallVector<ASTNode, 8> Elts(body->getElements().begin(),
+                                     body->getElements().end());
+        Elts.push_back(ASTNode(D));


When does this come up? I'm a little worried it could affect single-expression closures, but I guess those wouldn't have delayed decls in them. Still, a closure's body is ordered; adding something to the end of it isn't necessarily correct.

Thanks. I've added ordering to this code. What are you thinking w.r.t. single-expression closures? As far as when does this come up, I can run the tests again with an abort in there and let you know.

Honestly, I'm not sure if this really makes sense either. I think we just can't delay something out of a BraceStmt today, whether from a closure or not, and we shouldn't bother writing code to support it.

I inserted a forced assertion failure into this code and ran ninja check-swift-validation to see what happened. In the log file, my inserted assertion was triggered 8 times in:

IDE/crashers_fixed/030-swift-mangle-mangler-mangledecltype.swift

IDE/crashers_fixed/029-swift-decl-walk.swift

IDE/crashers_fixed/032-swift-expr-propagatelvalueaccesskind.swift

IDE/crashers_fixed/047-swift-typechecker-typechecktoplevelcodedecl.swift

IDE/crashers_fixed/080-swift-valuedecl-getinterfacetype.swift

IDE/crashers_fixed/084-swift-parser-consumedecl.swift

IDE/complete_crashes.swift

IDE/complete_in_closures.swift

In each case, it was running swift-ide-test.

So, it does happen in those contexts.

However, in each case, it's doing code completion, so this code would not be required until ASTScope lookup handles completion completely. I'm planning to land ASTScopeLookup with it initially disabled-by-default when completion is being done. So, I don't need this code right way.

Hmmm... What I can do is take it out of this PR, and put it in a radar for me to come back to in the future. That's probably the best course for now.

Assuming that you're surprised that this actually happens, join the club! I and I think @slavapestov and @DougGregor have also been surprised at some of the things going on with our AST!

Thanks for your help.

I suppose I'm not surprised it happens for completion, but I don't know what we're going to do about completion! Still, maybe it can be appended to the end but with an assertion that that's the right place for it. Anyway, this looks good without it for now.

jrose-apple · 2019-07-25T16:51:21Z

lib/Parse/ParseDecl.cpp

@@ -4879,6 +4879,10 @@ Parser::parseDeclVarGetSet(Pattern *pattern, ParseDeclOptions Flags,
  if (!PrimaryVar) {
    fillInAccessorTypeErrors(*this, accessors);
    Decls.append(accessors.Accessors.begin(), accessors.Accessors.end());
+    // Preserve the invariaent that accessor an can be found from its


Typo: "invariaent"

jrose-apple · 2019-07-25T16:51:50Z

lib/Parse/ParseDecl.cpp

+    // Preserve the invariaent that accessor an can be found from its
+    // VarDecl.
+    accessors.record(*this, storage, Invalid, Flags, StaticLoc, Attributes,
+                     TyLoc, /*indices*/ nullptr, Decls);


This seems reasonable to me, but @slavapestov or @rjmccall would know better if it's okay to call this here.

Thanks. Will check in with one of them. @slavapestov , you've been working with me on ASTScopes, what do you think?

davidungar · 2019-07-29T04:08:25Z

@swift-ci please smoke test os x platform

davidungar · 2019-07-29T22:23:18Z

@swift-ci please smoke test

jrose-apple

Looks good to me at this point, assuming the accessor thing is the right thing to do.

davidungar · 2019-07-29T22:40:06Z

@slavapestov , @rjmccall : @jrose-apple has suggested that I ask one of you to weigh in on the call to record which I believe I need for ASTScopes in parseDeclVarGetSet. Would one of you like to check that out? TIA.

davidungar · 2019-07-30T02:58:57Z

@jrose-apple is ok with this, AFAICT, so landing it now. Tx to all reviewers!

davidungar requested a review from harlanhaskins July 11, 2019 18:51

davidungar requested review from slavapestov, nkcsgexi, rintaro, benlangmuir and jrose-apple July 11, 2019 18:52

harlanhaskins reviewed Jul 11, 2019

View reviewed changes

benlangmuir reviewed Jul 11, 2019

View reviewed changes

jrose-apple reviewed Jul 12, 2019

View reviewed changes

benlangmuir approved these changes Jul 12, 2019

View reviewed changes

akyrtzi approved these changes Jul 12, 2019

View reviewed changes

jrose-apple reviewed Jul 12, 2019

View reviewed changes

David Ungar added 5 commits July 25, 2019 07:50

Fixes for ASTScope lookup

8caf6f3

1. Track real EndLoc of TypeAliasDecl and SubscriptDecl. 2. Ensure TypeAliasDecl gets added to AST when doing completion.

Fix SubscriptDecl end position

79b9f6c

Restore other test

87a4aa6

Oops! Missed porting over the line to set the alias decl end.

3c7de8d

Fixes for review

6577e68

David Ungar added 3 commits July 25, 2019 07:50

When delayed parsing a Decl in a closure body, add it to the body.

691b3c6

Ensure that an accessor can be found from its VarDecl.

008791f

Adapt to streamlined record.

9f60bb4

davidungar force-pushed the parser-changes branch from 2aa736f to 9f60bb4 Compare July 25, 2019 15:44

jrose-apple reviewed Jul 25, 2019

View reviewed changes

Insert in order and other fix

3076912

Remove code for updating closure body.

634a6eb

jrose-apple reviewed Jul 29, 2019

View reviewed changes

slavapestov approved these changes Jul 30, 2019

View reviewed changes

davidungar merged commit d236a17 into swiftlang:master Jul 30, 2019

[Parser] Fixes for ASTScope lookup #26095

[Parser] Fixes for ASTScope lookup #26095

Uh oh!

Conversation

davidungar commented Jul 11, 2019

Uh oh!

davidungar commented Jul 11, 2019

Uh oh!

davidungar commented Jul 11, 2019

Uh oh!

davidungar commented Jul 11, 2019

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!

davidungar commented Jul 11, 2019

Uh oh!

davidungar commented Jul 11, 2019

Uh oh!

Uh oh!

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!

jrose-apple left a comment

Choose a reason for hiding this comment

Uh oh!

davidungar commented Jul 12, 2019

Uh oh!

davidungar commented Jul 12, 2019

Uh oh!

davidungar commented Jul 12, 2019

Uh oh!

davidungar commented Jul 12, 2019

Uh oh!

davidungar commented Jul 12, 2019

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!

davidungar commented Jul 25, 2019

Uh oh!

davidungar commented Jul 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davidungar commented Jul 25, 2019

Uh oh!

davidungar commented Jul 25, 2019

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidungar commented Jul 25, 2019 •

edited

Loading