[SE-0491] Parse and ASTGen module selectors #84362

beccadax · 2025-09-17T23:33:32Z

This PR adds support for module selectors to the legacy parser and also updates ASTGen to lower the module selector-related syntax nodes introduced by swiftlang/swift-syntax#3091. Although the compiler parses module selectors with this PR, they don't have any actual effect on name lookup or other compiler behavior.

Makes progress on rdar://problem/19481048. Extracted from #34556. Currently under review as SE-0491.

Try to avoid calling `getSourceRange()` on a decl where that will form an invalid range.

When closing paren is missing, point at the location where it should be found, not the opening paren.

A PatternBindingEntry formed from a MissingPatternSyntax has no valid SourceLocs in it, and a PatternBindingDecl containing only PatternBindingEntries with no valid SourceLocs trips an assertion during availability checking. (Generating dummy SourceLocs can cause invalid overlaps between AvailabilityScopes, so that’s not a workable solution.) The fix is to: • Refuse to generate a PatternBindingEntry from a PatternBindingSyntax with a MissingPatternSyntax (MissingPatternSyntaxes at nested positions don’t have this problem) • Refuse to generate a PatternBindingDecl with no PatternBindingEntries This ensures that the invalid AST nodes are never formed. No current test cases hit this problem, but certain invalid module selector tests can run into this situation when run with ASTGen.

beccadax · 2025-10-07T18:04:22Z

@swift-ci smoke test

rintaro · 2025-10-10T15:36:46Z

lib/ASTGen/Sources/ASTGen/ASTGen.swift

  @inline(__always)
  func generateSourceLoc(_ node: some SyntaxProtocol) -> SourceLoc {
-    SourceLoc(at: node.positionAfterSkippingLeadingTrivia, in: self.base)
+    precondition(node.positionAfterSkippingLeadingTrivia.utf8Offset < 0x0000000100000000)


What is this for?

I think this was something I added to chase down a bug caused by missing tokens having very large, invalid offsets that turned into out-of-bounds SourceLocs. It probably doesn't belong in this PR; the constant here is a fairly arbitrary "big enough" value.

rintaro · 2025-10-10T15:44:43Z

lib/ASTGen/Sources/ASTGen/ASTGen.swift

+    arguments: (
+      leftParen: TokenSyntax,
+      labels: some Collection<TokenSyntax>,
+      rightParen: TokenSyntax
+    )? = Optional<(leftParen: TokenSyntax, labels: [TokenSyntax], rightParen: TokenSyntax)>.none


Can't this be just DeclNameArgumentsSyntax ?

Suggested change

arguments: (

leftParen: TokenSyntax,

labels: some Collection<TokenSyntax>,

rightParen: TokenSyntax

)? = Optional<(leftParen: TokenSyntax, labels: [TokenSyntax], rightParen: TokenSyntax)>.none

arguments: DeclNameArgumentsSyntax? = nil

Looks like it can be. Good catch!

rintaro · 2025-10-10T15:55:26Z

lib/ASTGen/Sources/ASTGen/Decls.swift

+      // The availability checker requires declarations to have valid SourceLocs, but MissingPatternSyntax lowers to
+      // an implicit AnyPattern with invalid SourceLocs. A top-level MissingPatternSyntax could therefore cause us to
+      // construct an invalid AST. Drop the whole binding instead.
+      // No need to diagnose; SwiftParser should have already diagnosed the `MissingPatternSyntax`.


It's a little unfortunate that we're missing possible diagnostics for the initializer expressions.
Can't we set the SourceLoc if the generated pattern is AnyPattern with invalid SourceLocs?

(Not sure MissingPatternSyntax + non-nil initializer is possible in practice. though)

If we rejiggered some things in the bridging layer, I suppose we could create an implicit AnyPattern with an explicit SourceLoc. Where would we get that SourceLoc from, though? The MissingPatternSyntax won't give us a usable one.

generateSourceLoc(pattern) would return the location of the next token of var/let or ,. I assume that is not usable because it would overwrap with the next decl? If so, generateSourceLoc(pattern.previousToken(viewMode: .sourceAccurate)) might work, but I'm fine with leaving this as-is for now.

lib/Parse/ParseDecl.cpp

rintaro · 2025-10-10T17:31:54Z

lib/Parse/ParseDecl.cpp

+      diagnose(Tok, diag::replace_module_selector_with_member_lookup)
+        .fixItReplace(Tok.getLoc(), ".");
+    }
+    HasNext = consumeIf(tok::period) || consumeIf(tok::colon_colon);


Could you align the implementation with SwiftParser?

rintaro · 2025-10-10T17:45:24Z

lib/Parse/ParseExpr.cpp

+  DeclNameRef nameRef =
+      parseDeclNameRef(nameLoc, diag::impossible_parse,
+                       DeclNameFlag::AllowAnonymousParamNames |
+                       DeclNameFlag::ModuleSelectorUnsupported);


Is this needed? I believe this is only called when Tok is tok::dollarident so there's no chance to have a module selector here.

Yeah, I could roll this back. It was more important back when parseDeclNameRef() always parsed a module selector and just diagnosed it when you passed ModuleSelectorUnsupported.

rintaro · 2025-10-10T17:51:54Z

lib/Parse/ParseExpr.cpp

+  if (P.Tok.is(tok::identifier) && P.peekToken().is(tok::colon_colon)) {
+    return P.lookahead(2, [&](auto &scope) {


How about making isAtModuleSelector() returning number of module selector tokens and

Suggested change

if (P.Tok.is(tok::identifier) && P.peekToken().is(tok::colon_colon)) {

return P.lookahead(2, [&](auto &scope) {

if (auto numTokens = P.isAtModuleSelector()) {

return P.lookahead(*numTokens, [&](auto &scope) {

rintaro · 2025-10-10T17:55:12Z

lib/Parse/ParseStmt.cpp

        //       the same as those of the result node, plus any junk consumed
        //       afterwards
-        auto Brace = BraceStmt::create(Context, Result.getStartLoc(),
+        auto Brace = BraceStmt::create(Context, StartLoc,


Any reason for this change?

IIRC, there are (invalid code) situations where Result.getStartLoc() will be an invalid SourceLoc. When BraceStmt::create is passed an invalid left brace location and a valid right brace location, calling Stmt::getSourceRange() will trip an assertion. Using StartLoc here fixes the issue because, like PreviousLoc, it is always valid.

rintaro · 2025-10-10T17:56:44Z

lib/Parse/ParseType.cpp

    break;
  case tok::kw_protocol:
-    if (startsWithLess(peekToken())) {
+    if (Tok.is(tok::kw_protocol) && startsWithLess(peekToken())) {


I don't think this is needed.

You're right; I think it was briefly needed until I factored out diagnoseAndRecover() into a helper lambda.

beccadax added 11 commits September 10, 2025 18:20

Use parseDeclNameRef in a few more places

2052a42

[NFC] Add DeclNameLoc to specialize/dynamicReplacement

2903835

[NFC] Make ASTDumper more defensive

9681117

Try to avoid calling `getSourceRange()` on a decl where that will form an invalid range.

[NFC] Tweak source loc of @_implements error

b59d388

When closing paren is missing, point at the location where it should be found, not the opening paren.

[NFC] Make pattern type checking more defensive

3cf4244

[NFC-ish] Do lookahead without llvm::function_ref

679c927

[NFC] Add tok::colon_colon to parser

d2e4867

Parse module selectors in permitted locations

ab30cc9

Allow :: as an alias for . in scoped imports

be19784

Update and test ASTGen for module selectors

fe22d37

beccadax requested review from CodaFi, DougGregor, bnbarham, hamishknight, hborla, rintaro, slavapestov and xedin as code owners September 17, 2025 23:33

beccadax requested a review from tshortli September 18, 2025 21:21

rintaro reviewed Oct 10, 2025

View reviewed changes

		if (P.Tok.is(tok::identifier) && P.peekToken().is(tok::colon_colon)) {
		return P.lookahead(2, [&](auto &scope) {

[SE-0491] Parse and ASTGen module selectors #84362

Are you sure you want to change the base?

[SE-0491] Parse and ASTGen module selectors #84362

Conversation

beccadax commented Sep 17, 2025

Uh oh!

beccadax commented Oct 7, 2025

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!

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!

beccadax Oct 10, 2025 • 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

beccadax Oct 10, 2025 •

edited

Loading