[NFC] Rework types and terms for imports' access paths #33716

beccadax · 2020-08-31T17:43:17Z

Currently, the use of the term "access path" in relation to imports is inconsistent. It sometimes refers to the entire chain of dotted identifiers, sometimes just to the part scoping an import to a declaration, and occasionally refers to the part that doesn't scope an import to a declaration. This is made worse by the use of typedef and using in many places, making semantics and invariants unclear.

This PR reforms the terminology surrounding the dotted chain of identifiers used in import statements, and introduces new, strong, better-documented types to avoid mistakes or confusion about them. Specifically:

An import path (the ImportPath type) represents an undifferentiated chain of identifiers in an import statement. By providing additional information about the ImportKind, it can be split into its constituent parts.
A module path (the ImportPath::Module type) represents the part of an import path that identifies the module or submodule being imported.
An access path (the ImportPath::Access type) represents the part of the import path that identifies the declaration targeted by a scoped import.

Each of these types has appropriate invariants about its length enforced, and a corresponding ::Builder type can be used to create a temporary. I have also added a constructor to split apart strings into Builder types, and refactored away some code in SourceKit that used to duplicate the ASTContext method for handling dotted module names.

beccadax · 2020-08-31T18:41:15Z

Review notes: This is one of those patches that changes a ton of code, but most of the change is fairly mechanical. Here is how I suggest you review this PR:

Read, skim, or skip the commits in order:
1. Read "[NFC] Move ImportKind to a new Import.h header"
2. Read "[NFC] Add new ImportPath types"
3. SKIM "[NFC] Adopt new ImportPath types and terminology" (I'll call out interesting bits later)
4. SKIM "[NFC] Rename “DeclPath” -> “AccessPath”"
5. Read "[NFC] Define “access path” in the lexicon"
6. Read "[NFC] Get rid of ASTContext::getModuleByName dups"
7. Read "[NFC] Add helper to parse import paths in strings"
8. Read "[NFC] Add ASTContext::getModuleByIdentifier()"
9. SKIP OVER commit "[NFC] Further simplify ModuleFile::associateWithFileContext" (I'll have you read the outcome later)
10. Read "[NFC] Delete obsolete AccessPathTy-based code"
Read the final versions of the code associated with the seven comments I've made below.

In addition to the actual reviewers I'm adding, I'll also tag a couple people in specific comments. I'd love it if they read more than just their specific sections, but I don't necessarily need them to.

beccadax · 2020-08-31T22:27:47Z

lib/IDE/CodeCompletion.cpp

+      // FIXME: Why are we allowing multiple module names? We don't support
+      // submodule qualification.
+      ImportPath::Module::Builder builder;
      for (auto Component : ITR->getComponentRange())
-        AccessPath.push_back({ Component->getNameRef().getBaseIdentifier(),
-                               Component->getLoc() });
-      if (auto Module = Context.getLoadedModule(AccessPath))
+        builder.push_back(Component->getNameRef().getBaseIdentifier(),
+                          Component->getLoc());
+      if (auto Module = Context.getLoadedModule(builder.get()))


I found the original code here confusing, so it's possible that I've misunderstood it somehow.

@brentdax I think you're right. It should probably just early exit with return false when there's more than one component here.

I've restructured this to skip this logic unless there is exactly one component in the IdentTypeRepr.

beccadax · 2020-08-31T22:29:11Z

lib/IDE/ModuleInterfacePrinting.cpp

-  auto LHSPath = LHS->getFullAccessPath();
-  auto RHSPath = RHS->getFullAccessPath();
+  // TODO: Probably buggy--thinks "import Foo" == "import Foo.Bar".
+  // ImportPathBase should provide universal comparison functions to avoid this.


I haven't added an SR number yet in case reviewers tell me that I'm wrong about this. (I don't want to make functional changes in this PR if possible, which is why I'm not just fixing the bug right away.)

Seems like a bug to me too.

https://bugs.swift.org/browse/SR-13490

I'll add the bug number to the comment so we don't lose track of it.

I've added the bug number to the comment.

beccadax · 2020-08-31T22:31:27Z

lib/IDE/SourceEntityWalker.cpp

-    return false;
+  if (Path.hasSubmodule())
+    if (!passModulePathElements(Path.takeParent(), ClangMod->Parent))
+      return false;


ImportPath::Module enforces an invariant that it has at least one module name in it, which required restructuring here. I think I've preserved the old logic, but it's hard to tell—I'm not sure when ClangMod can be nullptr or when Path could previously be empty.

This looks correct to me too. I think the empty Path handling was just related to the recursive call here, since it didn't have an equivalent of the hasSubmodule check before and would drop_back the last path component and get the non-existent parent of the corresponding ClangMod.

I've tweaked this a bit to flatten the control flow and enforce an invariant that a null ClangMod should never be passed in.

beccadax · 2020-08-31T22:33:41Z

lib/IRGen/IRGenDebugInfo.cpp

+  // ModuleDecl::ImportedModule inconsistently with typical use (placing the
+  // module path in the `accessPath` field). It's not immediately clear if there
+  // are matching inconsistencies elsewhere in IRGenDebugInfo.
+  ModuleDecl::ImportedModule Imported = { ImportPath::Access(), D->getModule() };


I don't think anything here reads the accessPath field (...so why are we passing ImportedModules around instead of just ModuleDecl*s?), but I could use an extra eye on this.

@adrian-prantl Can you comment on this?

This is used only in IRGenDebugInfoImpl::finalize() to avoid creating a debug info breadcrumb for a module import twice. If you are saying this use is inconsistent, I'd image that it likely doesn't work correctly at the moment. We probably didn't notice, since a duplicate import in the debug info won't cause any problems other than wasting space.

Can you remind me what the access path is in a typical module import example? This was never really clear to me since Swift uses a flat namespace for modules. Or is that changing now?

In a scoped import (like import class Foo.Bar), the access path is the declaration the import is scoped to (Bar). Most imports are not scoped (like import Foo); their access paths are empty.

If you’re trying to record what was imported so that you can fully(ish) recreate it later, you probably want to write D->getAccessPath() instead of ImportPath::Access() here. If you just want to record the modules and don’t care about the access paths, what’s here is fine.

Okay, that makes sense! At the moment, we only record imports on a module granularity in the debug info rdar://problem/68149326, so I think having an empty access path is actually what we'd want here!

Since you might eventually decide to start recording access paths, I think I'll just grab the right value from the ImportDecl and pass it along. Currently you'll just ignore it, but if that changes in the future you won't need to update this code.

I've force-pushed a change to construct it with the correct (ignored) access path.

lib/Serialization/ModuleFile.cpp

beccadax · 2020-08-31T22:40:38Z

utils/gyb_syntax_support/DeclNodes.py

+    # FIXME: technically misnamed; should be "ImportPathComponent"
    Node('AccessPathComponent', kind='Syntax',
         children=[
            Child('Name', kind='IdentifierToken'),
            Child('TrailingDot', kind='PeriodToken', is_optional=True),
         ]),

+    # FIXME: technically misnamed; should be "ImportPath"
    Node('AccessPath', kind='SyntaxCollection', element='AccessPathComponent'),


Fixing this will change user-visible names in SwiftSyntax; I'm not sure that's quite worth it.

beccadax · 2020-08-31T23:09:33Z

@nathawes I'd appreciate it if you could review the changes to lib/IDE and tools/SourceKit, and particularly the bits where I've left comments.

beccadax · 2020-08-31T23:17:54Z

I didn't see any breakage in downstream projects, but I don't quite believe that, so I'll start with a smoke test to confirm it.

@swift-ci please smoke test

beccadax · 2020-09-01T01:41:58Z

17:23:01 /home/buildnode/jenkins/workspace/swift-PR-Linux-smoke-test/branch-master/llvm-project/lldb/source/Plugins/Language/Swift/SwiftLanguage.cpp:1095:36: error: no type named 'AccessPathTy' in 'swift::ModuleDecl'
17:23:01                 swift::ModuleDecl::AccessPathTy access_path;
17:23:01                 ~~~~~~~~~~~~~~~~~~~^
17:23:01 1 error generated.

I knew it was too good to be true.

hamishknight

Neat!

lib/ClangImporter/ClangImporter.cpp

include/swift/AST/Import.h

nathawes

The SourceKit/IDE bits look good to me. Thanks!

beccadax · 2020-09-05T04:01:19Z

@swift-ci test

swift-ci · 2020-09-05T04:04:13Z

Build failed
Swift Test Linux Platform
Git Sha - eda10502399562199214d54be34e7f6abbad1607

swift-ci · 2020-09-05T04:05:54Z

Build failed
Swift Test OS X Platform
Git Sha - eda10502399562199214d54be34e7f6abbad1607

beccadax · 2020-09-08T19:46:05Z

@swift-ci please smoke test and merge

beccadax · 2020-09-09T00:56:19Z

@swift-ci please smoke test macOS platform

beccadax · 2020-09-09T17:49:19Z

@swift-ci please smoke test macOS platform

# Conflicts: # lib/IDE/CodeCompletion.cpp

To avoid ambiguity, ImportResolution and a few other things used the term “decl path” instead of “access path”. Switch back to the correct terminology now that the compiler is becoming more consistent about it.

This method was reimplemented three times in various source tools.

This is a reasonably common operation.

This was all deprecated in earlier commits.

beccadax · 2020-09-11T02:16:32Z

@swift-ci please smoke test and merge

beccadax · 2020-09-11T02:25:21Z

@swift-ci please smoke test and merge

beccadax force-pushed the access-path-denied branch 2 times, most recently from be19bf7 to 6ef321a Compare August 31, 2020 22:18

beccadax commented Aug 31, 2020

View reviewed changes

lib/Serialization/ModuleFile.cpp Show resolved Hide resolved

beccadax commented Aug 31, 2020

View reviewed changes

lib/Serialization/ModuleFile.cpp Show resolved Hide resolved

beccadax commented Aug 31, 2020

View reviewed changes

beccadax changed the title ~~[NFC] Rework types for access paths~~ [NFC] Rework types and terms for imports' access paths Aug 31, 2020

beccadax requested review from hamishknight, CodaFi, xymus and nathawes and removed request for xymus and nathawes August 31, 2020 23:01

beccadax marked this pull request as ready for review August 31, 2020 23:09

hamishknight approved these changes Sep 1, 2020

View reviewed changes

lib/ClangImporter/ClangImporter.cpp Outdated Show resolved Hide resolved

include/swift/AST/Import.h Outdated Show resolved Hide resolved

include/swift/AST/Import.h Show resolved Hide resolved

beccadax mentioned this pull request Sep 2, 2020

[NFC] Remove dead use of ModuleDecl::AccessPathTy swiftlang/llvm-project#1730

Merged

nathawes approved these changes Sep 2, 2020

View reviewed changes

xymus approved these changes Sep 2, 2020

View reviewed changes

beccadax mentioned this pull request Sep 2, 2020

[master-next] [NFC] Remove dead use of ModuleDecl::AccessPathTy swiftlang/llvm-project#1734

Merged

beccadax force-pushed the access-path-denied branch from 81b1fdf to eda8c15 Compare September 5, 2020 03:59

beccadax mentioned this pull request Sep 9, 2020

Fix non-public imported declaration diagnostic bug + Favor private imports during name lookup #33864

Merged

beccadax added 10 commits September 10, 2020 18:36

[NFC] Move ImportKind to a new Import.h header

5afe2f9

[NFC] Add new ImportPath types

1ed3cae

[NFC] Adopt new ImportPath types and terminology

cff4ddf

# Conflicts: # lib/IDE/CodeCompletion.cpp

[NFC] Rename “DeclPath” -> “AccessPath”

6da428a

To avoid ambiguity, ImportResolution and a few other things used the term “decl path” instead of “access path”. Switch back to the correct terminology now that the compiler is becoming more consistent about it.

[NFC] Define “access path” in the lexicon

f2249e9

[NFC] Get rid of ASTContext::getModuleByName dups

61f716d

This method was reimplemented three times in various source tools.

[NFC] Add helper to parse import paths in strings

7bfeeeb

[NFC] Add ASTContext::getModuleByIdentifier()

cce0411

This is a reasonably common operation.

[NFC] Further simplify ModuleFile::associateWithFileContext

4be67a6

[NFC] Delete obsolete AccessPathTy-based code

6f28536

This was all deprecated in earlier commits.

beccadax force-pushed the access-path-denied branch from eda8c15 to 6f28536 Compare September 11, 2020 02:09

swift-ci merged commit e9650cf into swiftlang:master Sep 11, 2020

beccadax deleted the access-path-denied branch September 11, 2020 06:04

beccadax mentioned this pull request Oct 24, 2023

[SR-13490] compareImports() in ModuleInterfacePrinting.cpp looks buggy #55932

Closed

[NFC] Rework types and terms for imports' access paths #33716

[NFC] Rework types and terms for imports' access paths #33716

Uh oh!

Conversation

beccadax commented Aug 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

beccadax commented Aug 31, 2020 • 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!

nathawes Sep 2, 2020 • 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!

beccadax Aug 31, 2020 • 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!

beccadax Sep 5, 2020 • 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!

beccadax Aug 31, 2020 • 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!

beccadax Aug 31, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

beccadax Aug 31, 2020 • 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!

beccadax commented Aug 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

beccadax commented Aug 31, 2020

Uh oh!

beccadax commented Sep 1, 2020

Uh oh!

hamishknight left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nathawes left a comment

Choose a reason for hiding this comment

Uh oh!

beccadax commented Sep 5, 2020

Uh oh!

beccadax commented Aug 31, 2020 •

edited

Loading

beccadax commented Aug 31, 2020 •

edited

Loading

nathawes Sep 2, 2020 •

edited

Loading

beccadax Aug 31, 2020 •

edited

Loading

beccadax Sep 5, 2020 •

edited

Loading

beccadax Aug 31, 2020 •

edited

Loading

beccadax Aug 31, 2020 •

edited

Loading

beccadax Aug 31, 2020 •

edited

Loading

beccadax commented Aug 31, 2020 •

edited

Loading