[NameLookup] ASTOOScope ontology #24594

davidungar · 2019-05-08T00:03:49Z

Initial cut at an ontology for scopes.
Disabled by default, but you can pass -enable-astscope-lookup to turn it on, or set an environment variable: export SWIFT_TEST_OPTIONS=-enable-astscope-lookup to tell lit to pass that option. There's also -Xfrontend -compare-to-astscope-lookup which will fail an assertion if the new lookup varies from the old.

Parser-based lookup is still used for now, even with the new lookup.

davidungar · 2019-05-08T03:16:32Z

@swift-ci please smoke test os x platform

davidungar · 2019-05-08T04:34:09Z

@swift-ci please smoke test os x platform

davidungar · 2019-05-09T21:26:56Z

@swift-ci please smoke test os x platform

davidungar · 2019-05-09T23:40:17Z

@swift-ci please smoke test os x platform

davidungar · 2019-05-10T00:55:52Z

@slavapestov @DougGregor Here's a PR with the scopes. This code definitely should be refactored before it lands. In particular, I'm not sure that the use of class templates worked that well because I could not get the method instantiations to work as I wanted them to. Also, it's possible that multiple inheritance could clean it up a lot. It does manage to find the right lookup decls and baseDC's for all of the .swift files in the test directory. I'm starting to work on the IndexOfFirstOuterResults: there's one test where it differs. Next, will be getting the dependency info to match, then seeing how many tests pass or fail with the new lookup code & also performance work.
Thanks!

davidungar · 2019-05-10T05:00:30Z

@swift-ci please smoke test os x platform

davidungar · 2019-05-10T05:01:33Z

At this point, the code seems to get the right answer for IndexOfFirstOuterResult, too. Next stop: the dependency info.

davidungar · 2019-05-26T18:22:21Z

@swift-ci please smoke test

davidungar · 2019-05-28T17:30:18Z

@swift-ci please smoke test os x platform

gottesmm · 2019-05-28T17:45:26Z

This is a huge patch, are you going to split this up?

gottesmm

Just some drive by comments. This is a really huge patch, it is important that it is split up. There is also a bunch of formatting (i.e. indentation) errors that would be caught by git-clang-format.

gottesmm · 2019-05-28T17:46:36Z

include/swift/AST/ASTScope.h

@@ -1,4 +1,5 @@
-//===--- ASTScope.h - Swift AST Scope ---------------------------*- C++ -*-===//
+//===--- ASTScopeImpl.h - Swift AST Object-Oriented Scope -----------*- C++


This is misformatted.

gottesmm · 2019-05-28T17:47:04Z

include/swift/AST/ASTScope.h

@@ -10,117 +11,75 @@
 //
 //===----------------------------------------------------------------------===//
 //
-// This file defines the ASTScope class and related functionality, which
+// This file defines the ASTScopeImpl class ontology, which


Top level file headers should have a \file and 3 slashes. See:

https://llvm.org/docs/CodingStandards.html#file-headers

Tx for the pointer. Fixed.

gottesmm · 2019-05-28T17:47:31Z

include/swift/AST/ASTScope.h

 //===----------------------------------------------------------------------===//
-#ifndef SWIFT_AST_AST_SCOPE_H
-#define SWIFT_AST_AST_SCOPE_H
+#ifndef SWIFT_AST_AST_OO_SCOPE_H


Please follow the normal way that we write these guards: file path. I.e.: SWIFT_AST_ASTSCOPE_H.

Tx. Slipped by when I did a global rename.

gottesmm · 2019-05-28T17:47:58Z

include/swift/AST/ASTScope.h

+class TypeAliasScope;
+class DeferredNodes;
+
+#pragma mark the root ASTScopeImpl class


We generally do not have pragma marks in our source code. Can you remove this when you are done.

I like the way they help my eyes parse the file. Also Xcode supports them. Is there a substitute?

actually looking at some other parts of the frontend, if no one else has issues with it, then I am cool with it (looks like @DougGregor is using it a bunch).

Great!
I could switch to // MARK if that's better

gottesmm · 2019-05-28T17:48:34Z

include/swift/AST/ASTScope.h

+
+namespace ast_scope {
+class ASTScopeImpl;
+class GTXScope;


What does GTX stand for? Can you expand this out? We prefer to avoid abbreviations unless it is something really commonly known (i.e. AST).

The Doxygen comment for GTXScope says GenericType or Extension Scope. I know it's a new abbreviation to the code base -- I've seen NTD used a bunch -- and I generally agree with spelling things out. If I spell it out, we get:
GenericTypeOrExtensionScope
GenericTypeOrExtensionWherePortion
GenericTypeOrExtensionWholePortion.
Seems to be getting a bit long, but I'm sympathetic to the idea of spelling it out.

I'll make sure that each class with GTX in its name includes a comment with an explanation of the abbreviation.

Please spell it out. How often are you going to read vs write this code (also since you use Xcode, I imagine the IDE will spell it out for you via autocomplete). For dinosaurs like me who do not use a new IDE, not having it spelled out makes life difficult.

I need to look at the other two ones. There may be a better name.

OK--spelled it out.

davidungar · 2019-05-28T18:04:19Z

@swift-ci please clean smoke test os x platform

davidungar · 2019-05-28T18:24:06Z

@gottesmm Thanks for taking a look! As far as splitting it up, the one piece that I can figure out to split off is the extra source location info for EditorPlaceHolder. Other than that, I don't see any way to split it up. It is code that is off-by-default, so I don't expect any destabilization. It is also code that is new, so I don't foresee any conflicts.

gottesmm

More feedback, some questions and some suggestions on stuff that I think could be split out of the patch.

gottesmm · 2019-05-28T18:30:54Z

include/swift/AST/Expr.h

@@ -4666,16 +4664,19 @@ class UnresolvedPatternExpr : public Expr {
 class EditorPlaceholderExpr : public Expr {
  Identifier Placeholder;
  SourceLoc Loc;
+  SourceLoc TrailingAngleBracketLoc;


Here is an example of something that could be ripped out into a separate patch. This could be sliced off.

Yes, agreed! Our comments crossed in the ether.

gottesmm · 2019-05-28T18:31:42Z

include/swift/AST/NameLookup.h

+  class ASTScopeImpl;
+  } // namespace ast_scope
+
+  /// LookupResultEntry - One result of unqualified lookup.


Why are you indenting this out, that just increases the diff without need.

Probably a git-clang-format thing--I'll fix.

I think it's fixed. Let me know if not. I have tried to git-format it all but may have missed something.

gottesmm · 2019-05-28T18:32:27Z

include/swift/AST/NameLookup.h

@@ -110,13 +150,13 @@ class UnqualifiedLookup {
  /// is used to determine which declarations in that body are visible.
  UnqualifiedLookup(DeclName Name, DeclContext *DC, LazyResolver *TypeResolver,
                    SourceLoc Loc = SourceLoc(), Options options = Options());
-
-  SmallVector<LookupResultEntry, 4> Results;
+  using ResultsVector = SmallVector<LookupResultEntry, 4>;


Are you using this type outside of the class? If not, why create this? If you are using it outside, it is better to use SmallVectorImpl.

Yes, UnqualifedLookupFactory uses it, and no, the Impl type won't do. But is there a better way?

Why wont it do. If you are passing it to someone, you can use SmallVectorImpl< LookupResultEntry> &

In unqualifiedLookup.cpp:

if (compareToASTScopes && useASTScopesForExperimentalLookupIfEnabled()) { ResultsVector results; size_t indexOfFirstOuterResult = 0; UnqualifiedLookupFactory scopeLookup(Name, DC, TypeResolver, Loc, options, results, indexOfFirstOuterResult);

The code needs to create a results vector, so it can make another factory for comparison purposes. Publishing the type seemed like the clearest way. I suppose the code could use a decltype instead but it's not clearer.

gottesmm · 2019-05-28T18:32:42Z

include/swift/AST/NameLookup.h

-
-  SmallVector<LookupResultEntry, 4> Results;
+  using ResultsVector = SmallVector<LookupResultEntry, 4>;
+  ResultsVector Results;
  /// The index of the first result that isn't from the innermost scope


Please put a newline line here.

Yup, good catch, thanks!

gottesmm · 2019-05-28T18:34:06Z

include/swift/AST/NameLookup.h

+                   NominalTypeDecl *const nominal,
+                   Optional<bool> isCascadingUse) = 0;
+
+  virtual void stopForDebuggingIfTargetLookup() = 0;


Is this going to be called in the code always? If this is only for debugging the compiler, it should be hidden behind an #ifdef.

Good point! Will do.

gottesmm · 2019-05-28T18:42:38Z

lib/AST/ASTScopeCreation.cpp

+  if (se)
+    ScopeCreator().visit(se, this, deferred);
+}
+void ASTScopeImpl::dispatchAndCreateIfNeeded(Decl *d, DeferredNodes &deferred) {


Can you make sure to put spaces in between things like this?

Yes, a space there is good, thanks. Other places, I'm not so sure:

ASTScopeImpl * ExtensionScope::createTrailingWhereClauseScope(ASTScopeImpl *parent) { return parent->createSubtree2D<ExtensionScope, GTXWherePortion>(decl); } ASTScopeImpl * NominalTypeScope::createTrailingWhereClauseScope(ASTScopeImpl *parent) { return parent->createSubtree2D<NominalTypeScope, GTXWherePortion>(decl); } ASTScopeImpl * TypeAliasScope::createTrailingWhereClauseScope(ASTScopeImpl *parent) { return parent->createSubtree2D<TypeAliasScope, GTXWherePortion>(decl); }

Here, I could use a define, but it seems not quite an improvement for such a small number. So, I go with tight repetition.

gottesmm · 2019-05-28T18:43:11Z

lib/AST/ASTScopeCreation.cpp

+  // If there's another conditional clause, add it as the child.
+  if (index + 1 < getContainingStatement()->getCond().size())
+    createSubtreeForNextConditionalClause(deferred);
+  else {


Everywhere else you did if () { } else { }. Can you be consistent with that here.

Yes, I see where I used braces but should not have. Thanks. I'm following a guideline from @jordan, which I quite like:
If the statement in the if (etc) is visually short (typ. one line) then braces just add clutter.
But if the statement wraps, then braces are better.

I'll go through the code and eliminate bogus braces.

All I want is for you to be consistent about it.

Quite right and fair!

gottesmm · 2019-05-28T18:43:22Z

lib/AST/ASTScopeCreation.cpp

+  createSubtree<ClosureBodyScope>(captureList, closureExpr);
+}
+
+


Another inconsistency around newlines.

Yup, fixed that one. But there will be places where I think that an extra newline can increase readability, just not there! Thanks.

gottesmm · 2019-05-28T18:44:06Z

lib/AST/ASTScopeCreation.cpp

+
+  public:
+    ClosureFinder(
+        llvm::function_ref<void(NullablePtr<CaptureListExpr>, ClosureExpr *)>


Just a friendly FYI, I injected function_ref into the swift namespace a bit ago. You can drop the llvm::.

Cool! I'll fix all my uses.

gottesmm · 2019-05-28T18:44:19Z

lib/AST/ASTScopeCreation.cpp

+        : foundClosure(foundClosure) {}
+
+    std::pair<bool, Expr *> walkToExprPre(Expr *E) override {
+      if (auto closure = dyn_cast<ClosureExpr>(E)) {


Can you put a * on the pointer auto to make it clear it is a pointer.

Good catch, thanks!

davidungar · 2019-05-28T18:46:03Z

@swift-ci please clean smoke test os x platform

davidungar · 2019-06-11T03:27:21Z

@swift-ci please smoke test os x platform

davidungar · 2019-06-14T04:42:48Z

@swift-ci please smoke test

davidungar · 2019-06-14T07:06:38Z

@swift-ci please smoke test Linux platform

davidungar · 2019-06-14T15:04:00Z

Merged per offline conversation with @slavapestov

compnerd · 2019-06-15T16:39:29Z

This breaks on Windows! https://compnerd.visualstudio.com/windows-swift/_build/results?buildId=3205

…tree This is swift-5.1-branch only fix. The fix on master has been superseded by this pull request: swiftlang#24594 rdar://52128292

davidungar force-pushed the A5-7-ASTOOScope-rebased branch from 05a1fd9 to b48c168 Compare May 8, 2019 00:11

davidungar requested review from slavapestov and DougGregor May 10, 2019 00:52

davidungar force-pushed the A5-7-ASTOOScope-rebased branch from ffaec97 to 1d41fb5 Compare May 26, 2019 18:22

davidungar force-pushed the A5-7-ASTOOScope-rebased branch 2 times, most recently from 9979bfd to c3da9c4 Compare May 28, 2019 17:15

davidungar changed the title ~~DNM [NameLookup] ASTOOScope ontology~~ [NameLookup] ASTOOScope ontology May 28, 2019

davidungar force-pushed the A5-7-ASTOOScope-rebased branch from c3da9c4 to 40d811e Compare May 28, 2019 17:29

ASTOOScope ontology

663760e

davidungar force-pushed the A5-7-ASTOOScope-rebased branch from 40d811e to 663760e Compare May 28, 2019 17:48

gottesmm reviewed May 28, 2019

View reviewed changes

David Ungar added 2 commits May 28, 2019 10:55

Clean up

c7a6f80

Only need NameLookup for the ASTScope interface class.

173bc09

Some review tweaks.

3f820c3

Typo fix.

741a0bf

gottesmm reviewed May 28, 2019

View reviewed changes

Review cleanups.

6942344

David Ungar added 2 commits June 10, 2019 19:47

Fix scope test

6b6c983

Passes either way

2d0a8f2

David Ungar added 19 commits June 11, 2019 09:08

add -disable-parser-lookup

6cfcabd

Use unique_ptr for Scope, getScope returns a reference

69f6b41

invariant

884648d

Outline expandInContext

916b217

Rename ASTNode_or_DeclPtr

21905c1

Rephrase comment

9b40b39

Typos, etc.

55323bb

Outline lookInGenericsAndSelfType

4122438

Fix

b418588

Explain source ranges

16ca723

Better method scope lookup

2f2bbfb

Outer generics

72b2096

Clean up

3e95844

Added fixmes

8988fa9

implicit checks

6f6a772

Rm doISplitAScope for visitBraceStmt

40cd518

Rm redundant computeSelfDCForParemt

44c6248

cosmetic

50358b2

typo

320dd89

davidungar merged commit 06527db into swiftlang:master Jun 14, 2019

nkcsgexi mentioned this pull request Jun 27, 2019

[5.1] Parser: avoid emitting disabled var diagnostics when building syntax tree #25838

Merged

davidungar deleted the A5-7-ASTOOScope-rebased branch September 23, 2019 16:17

		@@ -1,4 +1,5 @@
		//===--- ASTScope.h - Swift AST Scope ---------------------------- C++ --===//
		//===--- ASTScopeImpl.h - Swift AST Object-Oriented Scope -----------*- C++

[NameLookup] ASTOOScope ontology #24594

[NameLookup] ASTOOScope ontology #24594

Uh oh!

Conversation

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

Uh oh!

Uh oh!

davidungar commented May 8, 2019

Uh oh!

davidungar commented May 8, 2019

Uh oh!

davidungar commented May 9, 2019

Uh oh!

davidungar commented May 9, 2019

Uh oh!

davidungar commented May 10, 2019

Uh oh!

davidungar commented May 10, 2019

Uh oh!

davidungar commented May 10, 2019

Uh oh!

davidungar commented May 26, 2019

Uh oh!

davidungar commented May 28, 2019

Uh oh!

gottesmm commented May 28, 2019

Uh oh!

gottesmm left a comment

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!

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 May 28, 2019

Uh oh!

davidungar commented May 28, 2019

Uh oh!

gottesmm left a comment

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

davidungar commented May 8, 2019 •

edited

Loading