Describe a missing decl with attributes or modifiers as "declaration"#3338
Open
broken-circle wants to merge 1 commit into
Open
Describe a missing decl with attributes or modifiers as "declaration"#3338broken-circle wants to merge 1 commit into
broken-circle wants to merge 1 commit into
Conversation
When producing a description for a single `MissingDeclSyntax` that already carries attributes or modifiers, the parser has committed to it being a declaration, so describe it that way directly. The general climb logic in `nodesDescriptionAndCommonParent(_:)` would otherwise pick up the surrounding container's slot name (e.g. `"statements"` for a code block), which is accurate to the grammar but misleading as a description of the missing element.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
MissingNodesErroralready recognizes aMissingDeclSyntaxcarrying attributes or modifiers as a declaration, andafterClauseuses that signal to produce tailored diagnostics. This PR extends the same recognition to the description half of the message, which is currently computed independently and can disagree.For example:
The misleading description comes from the climb in the
.nodecase ofNodesDescriptionPart.description(format:), which walks up through single-child parents looking for achildNameInParent. For aMissingDeclSyntaxalone in a function body's code block, the chainMissingDecl → CodeBlockItem → CodeBlockItemListconsists entirely of single-child parents, so the climb reachesCodeBlockItemListand returns its slot name"statements". For the sameMissingDeclSyntaxat the top level alongside other statements, the surroundingCodeBlockItemListhas more than one child, the climb terminates early, and the fallbacknodeTypeNameForDiagnostics(allowBlockNames:)returns"declaration". The diagnostic wording therefore depends on whether other items happen to sit alongside the missing one, which is structural noise unrelated to what the user wrote.After this PR, the code above produces an
expected declarationdiagnostic and aninsert declarationfix-it.Alternatives considered
Fix the climb logic
A general fix to the climb logic may involve annotating each collection-typed child's
nameForDiagnosticswith information regarding whether it names the aggregate (e.g."statements","members") or an element (e.g."condition"forConditionElementListSyntaxin aguard). This could be something likeenum NamingKind, and would be plumbed intochildNameForDiagnostics(_:)to be consulted during the climb. This is probably the principled solution, but it crosses into code-generation and substantially expands the PR's scope for currently just one FIXME.Never take a collection name
The
childNameInParentof aSyntaxCollectionnames the collection slot (e.g."statements","elements","arguments"), not any individual element inside it. When describing a single missing element that happens to be the only thing in such a collection, that name is sometimes misleading, since it would describe the missing thing as if it were the whole list. This change would treat collections as transparent in the climb, so we either find a more specific named slot above, or fall through to the node's own type name.This is implemented with a one-line change in the
.nodecase ofNodesDescriptionPart.description(format:):var walk: Syntax = node while true { + if !walk.kind.isSyntaxCollection, let childName = walk.childNameInParent { - if let childName = walk.childNameInParent { return childName } if let parent = walk.parent, parent.children(viewMode: .all).count == 1 { walk = parent } else { break } }However, the
childNameInParentof aSyntaxCollectionisn't always an aggregate name. Some collections (e.g.ConditionElementListSyntaxin aguardstatement) carry an element-naming slot name like"condition", which is the correct description for a missing element inside them. The climb has no way to tell these two kinds apart: both are simplynameForDiagnostics: Stringwith nothing further to discriminate on, so a general "skip-collections" rule regresses the element-naming cases. Concretely, with this change applied,GuardTests.testGuard2()regresses fromexpected conditiontoexpected expression, which is accurate but less useful, and inconsistent with the correspondingguard/if/while/repeattests.