[Parse] Discriminate local variables #16130

rintaro · 2018-04-24T12:38:08Z

Set local discriminator for all local VarDecls. Otherwise, they cannot be discriminated with USRs. This change is needed for rename refactoring which uses USR for discrimiating variable names.

SR-7205, rdar://problem/34701880

rintaro · 2018-04-24T12:38:16Z

@swift-ci Please smoke test

rintaro · 2018-04-24T13:37:57Z

test/refactoring/rename/Outputs/local/catch_1.swift.expected

+  do {
+    try arg()
+  } catch let x as Err1 {
+    print(xRenamed)


let x above should be renamed as well. Investigating.

rintaro · 2018-04-25T03:58:20Z

@swift-ci Please smoke test

rintaro · 2018-04-25T06:28:54Z

lib/IDE/SwiftSourceDocInfo.cpp

@@ -581,7 +581,7 @@ std::pair<bool, Pattern*> NameMatcher::walkToPatternPre(Pattern *P) {
  if (isDone() || shouldSkip(P->getSourceRange()))
    return std::make_pair(false, P);

-  tryResolve(ASTWalker::ParentTy(P), P->getLoc());
+  tryResolve(ASTWalker::ParentTy(P), P->getStartLoc());


@nkcsgexi is this OK?

I think this is fine for patterns. We need to be careful for decls though, sometimes getLoc() and getStartLoc point to different position IIRC.

rintaro · 2018-04-25T06:37:55Z

@rjmccall In commit message of 892a900, you explicitly excluded physical vars

Track a discriminator for named declarations (except physical vars) in local contexts, for future use in mangling.

What was the reason?

rjmccall · 2018-04-25T15:46:27Z

Computing and storing a discriminator means time and memory overhead, and we don't actually need a discriminator because an ordinary local variable doesn't require the creation of anything outside the code-generation of the function.

rjmccall · 2018-04-25T15:50:14Z

If you're really going to attach discriminators to every single local variable, the side-table storage approach is probably unreasonable.

rintaro · 2018-05-08T12:24:36Z

I embedded LocalDiscriminator to ValueDecls class fields.
@rjmccall How does this look?

JustFYI, I measured how many local discriminators are actually set, using Swift stdlib source files.

Before:
Parser::setLocalDiscriminator is called 5683 times, 130 are actually in local context.
After:
Parser::setLocalDiscriminator is called 10015 times, 3861 are actually in local context.

Focused on VarDecls:
Before:
Parser::setLocalDiscriminator is called 1275 times, 0 are actually in local context.
After:
Parser::setLocalDiscriminator is called 5607 times, 3731 are actually in local context.

rintaro · 2018-05-08T12:24:43Z

@swift-ci Please smoke test

jrose-apple · 2018-05-08T17:33:49Z

I see you removing the local discriminator from serialization completely. Is it really not needed?

rintaro · 2018-05-08T17:46:08Z

@jrose-apple I believe it's not needed because local discriminators are serialized/deserialized as a part of Decl emission for every ValueDecls in local context. https://github.com/apple/swift/blob/db423c32821a7223a13cdf4786204cf843aa2459/lib/Serialization/Serialization.cpp#L2662-L2667

jrose-apple · 2018-05-08T17:54:13Z

Ah, I didn't realize that was already there! Okay then. ~~Please bump the version number in ModuleFormat.h too!~~ Oops, you got that too. Thank you.

rjmccall · 2018-05-08T20:49:15Z

Do we really only have 3700 local variable declarations in the stdlib? That seems extremely low.

Are we not setting a discriminator on parameters? I guess maybe we don't need to because we can identify them reliably relative to the function they're parameters of; that would definitely keep the count down.

rintaro · 2018-05-09T08:41:30Z

Ah, I forgot about ParamDecl. And yes, we need to discriminate them.

func foo(_ arg: Int) {
  var arg = arg;
}

Updated the PR.
Now that, 10492 out of 12368 VarDecls are in local context.

As a side note, this PR impacts the parse time by around 1% (0.654s -> 0.662s) and max rss by around 1% (165,376,000 -> 167,411,712).

rintaro · 2018-05-09T09:51:35Z

@swift-ci Please smoke test

rintaro · 2018-05-09T12:54:09Z

@swift-ci Please smoke test

rjmccall · 2018-05-09T16:54:50Z

Are you sure you can't distinguish ParamDecls implicitly? A ParamDecl should always be uniquely identified by (1) the function it's a parameter of, (2) the fact that it's a parameter, and (3) its index.

rjmccall · 2018-05-09T16:55:28Z

Not that it really hurts to discriminate them, given that we're paying the memory cost in the base class anyway.

rintaro · 2018-05-09T22:54:19Z

Distinguishing ParamDecls without using LocalDiscriminator is technically feasible, but I think using LocalDiscriminator is easiest/convenient way to achieve that. And, as you said, we already have facility for that.
@swift-ci Please smoke test

rintaro · 2018-05-10T00:34:03Z

@swift-ci Please smoke test

rintaro · 2018-05-10T08:43:10Z

@rjmccall Could you approve this?

rjmccall

This PR looks good to me, but I think you should consider adding some way to test local discriminators more directly. Maybe a testing switch that prints declaration names with their line, column, and discriminator?

Set local discriminator for all local `VarDecl`s. Otherwise, they cannot be discriminated with USRs. This change is needed for rename refactoring which uses USR for discrimiating variable names. https://bugs.swift.org/browse/SR-7205, rdar://problem/34701880

For Example `getLoc()` for `foo as Ty`(`IsPattern`) is at 'as'. We should try to resolve this at position of 'foo'.

Local discriminators are emitted as a part of decl.

We have to discriminate between params and local variables.

rintaro · 2018-05-11T06:45:23Z

Thanks. That is actually a good idea!

rintaro · 2018-05-11T06:45:30Z

Resolved conflicts.
@swift-ci Please smoke test

rintaro commented Apr 24, 2018

View reviewed changes

rintaro commented Apr 25, 2018

View reviewed changes

rintaro force-pushed the parse-var-discriminator branch 2 times, most recently from 95f0475 to 3b19a00 Compare May 8, 2018 12:00

rintaro force-pushed the parse-var-discriminator branch from 3b19a00 to d086445 Compare May 9, 2018 08:35

rjmccall approved these changes May 10, 2018

View reviewed changes

rintaro added 5 commits May 11, 2018 15:34

[IDE] Use getStartLoc() for matching pattern ranges

dc76149

For Example `getLoc()` for `foo as Ty`(`IsPattern`) is at 'as'. We should try to resolve this at position of 'foo'.

[AST] Stop using side-table for local discriminators

9fc0985

[Serialization] Omit local discrimiator from LocalDeclTableInfo

6496175

Local discriminators are emitted as a part of decl.

[Parser] Set local discriminator to ParamDecls

a6f7a8e

We have to discriminate between params and local variables.

rintaro force-pushed the parse-var-discriminator branch from d086445 to a6f7a8e Compare May 11, 2018 06:44

rintaro merged commit 9dcd0f9 into swiftlang:master May 11, 2018

rintaro deleted the parse-var-discriminator branch May 11, 2018 08:30

rintaro mentioned this pull request May 11, 2018

[4.2] Discriminate local variables #16536

Merged

rintaro mannequin mentioned this pull request May 7, 2018

[SR-7205] Unified Symbol Resolution giving non-unique resolution of variables #49753

Closed

[Parse] Discriminate local variables #16130

[Parse] Discriminate local variables #16130

Uh oh!

Conversation

rintaro commented Apr 24, 2018

Uh oh!

rintaro commented Apr 24, 2018

Uh oh!

rintaro Apr 24, 2018

Choose a reason for hiding this comment

Uh oh!

rintaro commented Apr 25, 2018

Uh oh!

rintaro Apr 25, 2018

Choose a reason for hiding this comment

Uh oh!

nkcsgexi Apr 25, 2018

Choose a reason for hiding this comment

Uh oh!

rintaro commented Apr 25, 2018

Uh oh!

rjmccall commented Apr 25, 2018

Uh oh!

rjmccall commented Apr 25, 2018

Uh oh!

rintaro commented May 8, 2018

Uh oh!

rintaro commented May 8, 2018

Uh oh!

jrose-apple commented May 8, 2018

Uh oh!

rintaro commented May 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jrose-apple commented May 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rjmccall commented May 8, 2018

Uh oh!

rintaro commented May 9, 2018

Uh oh!

rintaro commented May 9, 2018

Uh oh!

rintaro commented May 9, 2018

Uh oh!

rjmccall commented May 9, 2018

Uh oh!

rjmccall commented May 9, 2018

Uh oh!

rintaro commented May 9, 2018

Uh oh!

rintaro commented May 10, 2018

Uh oh!

rintaro commented May 10, 2018

Uh oh!

rjmccall left a comment

Choose a reason for hiding this comment

Uh oh!

rintaro commented May 11, 2018

Uh oh!

rintaro commented May 11, 2018

Uh oh!

Uh oh!

rintaro commented May 8, 2018 •

edited

Loading

jrose-apple commented May 8, 2018 •

edited

Loading