Skip to content
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

[NFC] Distinguish references to names from declarations of those names by type #27683

Merged
merged 16 commits into from
Dec 11, 2019

Conversation

beccadax
Copy link
Contributor

@beccadax beccadax commented Oct 15, 2019

This is a refactoring in support of the module qualification feature I've previously pitched on Evolution.

The eventual goal for that feature is to allow any place that uses a name for a declaration defined elsewhere to qualify it with a module name. That is, all of these should allow modules:

print(SomeModule::x)   // The `x` visible from SomeModule
({ () -> SomeModule::T in ... })(x)   // The `T` from SomeModule
T.SomeModule::init()   // The `T.init()` declared in SomeModule

But things like this should not:

func SomeModule::fn() { ... }   // defining a function in a different module???
let SomeModule::x = 1    // defining a constant in another module???

To support this distinction, this PR adds a DeclNameRef type to the compiler. This type currently just wraps DeclName, but in the future it will also be able to store a module name when the user module-qualifies a name. Declarations will continue to store a DeclName or Identifier for their own names, but name lookup entry points—and therefore many other AST nodes like UnresolvedDeclRefExprs and TypeReprs—will switch over to using DeclNameRef. Thus, this PR will establish the pathways through which module qualification information will flow without actually making any functional changes to the compiler. This will reduce the amount of merge conflicts during the feature's design and review.

While this PR is in progress, DeclNameRef's constructors will be implicit but deprecated, and a DeclNameRef_ function is used whenever we want to explicitly construct a DeclNameRef. Before I merge this commit, I will make the DeclNameRef constructors explicit, rename all DeclNameRef_s to DeclNameRef, and delete the DeclNameRef function.

@beccadax beccadax force-pushed the i-understood-that-reference branch 4 times, most recently from fc03843 to 2a8e86e Compare November 9, 2019 04:02
@beccadax

This comment has been minimized.

@beccadax

This comment has been minimized.

@swift-ci

This comment has been minimized.

@swift-ci

This comment has been minimized.

@beccadax

This comment has been minimized.

@swift-ci

This comment has been minimized.

@swift-ci

This comment has been minimized.

@beccadax
Copy link
Contributor Author

beccadax commented Dec 6, 2019

I'm going to try running tests from the llvm-project side in swiftlang/llvm-project#430.

@beccadax beccadax marked this pull request as ready for review December 10, 2019 19:03
@beccadax
Copy link
Contributor Author

With swiftlang/llvm-project#430

@swift-ci please test

@beccadax beccadax changed the title [WIP] Distinguish references to names from declarations of those names by type Distinguish references to names from declarations of those names by type Dec 10, 2019
@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - e9f61145b10edd43c1d6893e88193a1346b11e0d

@beccadax
Copy link
Contributor Author

Notes for reviewers:

  • This is mostly boring NFC type name changes.

  • In general, DeclNameRef should be used where the developer has written the name of a type that is declared elsewhere. However, at various points we need to take a name from a declaration and look it up as though the user wrote it, and at other points we need to strip away the distinction between names and references to names so we can work with them as equals, and there are some places in constraint solving particularly where we end up losing the original names, so you'll sometimes see conversions back and forth.

  • In many of the commits, you'll see DeclNameRef_. These mark places where I eventually inserted an explicit conversion. There are no DeclNameRef_s left in the final commit, and the function they used to call is gone.

  • The easiest thing to do might be to read the smaller commits in more detail and skim the large commits for parts that look non-trivial.

Copy link
Contributor

@CodaFi CodaFi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't spot anything wrong, but then again this is a lot of code. Can the smaller refactorings be merged off separately? Ideally this patch would just be the renaming and the type plumbing afterwards.

@beccadax
Copy link
Contributor Author

@CodaFi I could pull b1fbdbfe (Miscellaneous improvements to Playground and PCMacro transforms) into a separate followup PR; that part is +84 -88 and nothing else needs it. I don't know if there's anything else I can easily extract, though. Any suggestions?

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - e9f61145b10edd43c1d6893e88193a1346b11e0d

@beccadax
Copy link
Contributor Author

With swiftlang/llvm-project#430

@swift-ci please test

@swift-ci

This comment has been minimized.

@swift-ci

This comment has been minimized.

We were unnecessarily converting back and forth between base names and full names, which seemed a bit silly.
This change adds UnresolvedDotExpr::createImplicit() and UnresolvedDeclRefExpr::createImplicit() helpers. These calls simplify several tedious bits of code synthesis that would otherwise become even more tedious with DeclNameRef in the picture.
This has the side effect of threading compound name support through more enum pattern code, although it’s still not complete.
So that DeclName can have a spare bit, allowing it to be used in places where we’re currently using PointerIntPair<Identifier, 1> or PointerUnion<Identifier, Whatever>.
Replaces `ComponentIdentTypeRepr::getIdentifier()` and `getIdLoc()` with `getNameRef()` and `getNameLoc()`, which use `DeclName` and `DeclNameRef` respectively.
This feature is barely used and needs refactoring that would be outside the scope of this series of commits (see https://bugs.swift.org/browse/SR-11241).
Change the various AST transforms to use prebuilt DeclName constants more heavily rather than an ad-hoc mix of Identifiers, string literals, std::strings, and StringRefs with questionable memory management.
This type wraps a DeclName, indicating that it is a reference to a declaration that exists somewhere else and it requires slightly “fuzzy” comparison (i.e. if it’s not compound, only the base names should be compared). DeclName::matchesRef() and MemberLookupTable::find() both now take a DeclNameRef instead of a DeclName.

This commit temporarily allows implicit conversion from DeclName; I’ll flip the switch on that in a later commit.
This huge commit contains as many of the mechanical changes as possible.
Seems to have been a mismerge or something?
@beccadax
Copy link
Contributor Author

Needed yet another rebase.

With swiftlang/llvm-project#430

@swift-ci please smoke test

@beccadax
Copy link
Contributor Author

With swiftlang/llvm-project#430

@swift-ci please test compiler performance

@beccadax
Copy link
Contributor Author

With swiftlang/llvm-project#430

@swift-ci please test source compatibility

@beccadax
Copy link
Contributor Author

With swiftlang/llvm-project#430

@swift-ci please smoke test compiler performance

@swift-ci
Copy link
Contributor

Summary for master smoketest

Unexpected test results, excluded stats for ReactiveCocoa

No regressions above thresholds

Debug

debug brief

Regressed (0)
name old new delta delta_pct
Improved (0)
name old new delta delta_pct
Unchanged (delta < 1.0% or delta < 100.0ms) (3)
name old new delta delta_pct
Frontend.NumInstructionsExecuted 105,231,041,153 105,177,753,206 -53,287,947 -0.05%
LLVM.NumLLVMBytesOutput 6,147,372 6,147,372 0 0.0%
time.swift-driver.wall 8.9s 9.0s 66.9ms 0.75%

debug detailed

Regressed (0)
name old new delta delta_pct
Improved (0)
name old new delta delta_pct
Unchanged (delta < 1.0% or delta < 100.0ms) (18)
name old new delta delta_pct
AST.NumLoadedModules 660 660 0 0.0%
AST.NumTotalClangImportedEntities 3,666 3,666 0 0.0%
IRModule.NumIRBasicBlocks 17,929 17,929 0 0.0%
IRModule.NumIRFunctions 10,597 10,597 0 0.0%
IRModule.NumIRGlobals 8,820 8,820 0 0.0%
IRModule.NumIRInsts 309,125 309,125 0 0.0%
IRModule.NumIRValueSymbols 18,441 18,441 0 0.0%
LLVM.NumLLVMBytesOutput 6,147,372 6,147,372 0 0.0%
SILModule.NumSILGenFunctions 5,372 5,372 0 0.0%
SILModule.NumSILOptFunctions 7,270 7,270 0 0.0%
Sema.NumConformancesDeserialized 11,782 11,782 0 0.0%
Sema.NumConstraintScopes 38,998 38,998 0 0.0%
Sema.NumDeclsDeserialized 115,226 115,226 0 0.0%
Sema.NumFunctionsTypechecked 2,034 2,034 0 0.0%
Sema.NumGenericSignatureBuilders 4,068 4,068 0 0.0%
Sema.NumLazyIterableDeclContexts 18,531 18,531 0 0.0%
Sema.NumTypesDeserialized 45,676 45,676 0 0.0%
Sema.NumTypesValidated 5,918 5,918 0 0.0%

Release

release brief

Regressed (0)
name old new delta delta_pct
Improved (0)
name old new delta delta_pct
Unchanged (delta < 1.0% or delta < 100.0ms) (3)
name old new delta delta_pct
Frontend.NumInstructionsExecuted 147,629,646,984 147,643,148,408 13,501,424 0.01%
LLVM.NumLLVMBytesOutput 6,863,584 6,863,576 -8 -0.0%
time.swift-driver.wall 17.0s 17.1s 121.4ms 0.71%

release detailed

Regressed (0)
name old new delta delta_pct
Improved (0)
name old new delta delta_pct
Unchanged (delta < 1.0% or delta < 100.0ms) (18)
name old new delta delta_pct
AST.NumLoadedModules 76 76 0 0.0%
AST.NumTotalClangImportedEntities 2,136 2,136 0 0.0%
IRModule.NumIRBasicBlocks 17,957 17,957 0 0.0%
IRModule.NumIRFunctions 9,912 9,912 0 0.0%
IRModule.NumIRGlobals 8,921 8,921 0 0.0%
IRModule.NumIRInsts 200,046 200,046 0 0.0%
IRModule.NumIRValueSymbols 18,011 18,011 0 0.0%
LLVM.NumLLVMBytesOutput 6,863,584 6,863,576 -8 -0.0%
SILModule.NumSILGenFunctions 4,178 4,178 0 0.0%
SILModule.NumSILOptFunctions 6,229 6,229 0 0.0%
Sema.NumConformancesDeserialized 11,521 11,521 0 0.0%
Sema.NumConstraintScopes 38,800 38,800 0 0.0%
Sema.NumDeclsDeserialized 32,115 32,115 0 0.0%
Sema.NumFunctionsTypechecked 2,034 2,034 0 0.0%
Sema.NumGenericSignatureBuilders 1,579 1,579 0 0.0%
Sema.NumLazyIterableDeclContexts 4,267 4,267 0 0.0%
Sema.NumTypesDeserialized 16,745 16,745 0 0.0%
Sema.NumTypesValidated 3,914 3,914 0 0.0%

@beccadax beccadax merged commit 7543a89 into swiftlang:master Dec 11, 2019
beccadax added a commit to swiftlang/llvm-project that referenced this pull request Dec 11, 2019
LLDB update for swiftlang/swift#27683 "Distinguish references to names from declarations of those names by type"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants