Reparse non-identifier jsdoc names where possible, error otherwise#4058
Open
weswigham wants to merge 6 commits into
Open
Reparse non-identifier jsdoc names where possible, error otherwise#4058weswigham wants to merge 6 commits into
weswigham wants to merge 6 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adjusts the JSDoc reparse logic so that non-identifier names from JSDoc tags either get rewritten into something emittable (string-literal property names for @property, sanitized identifier names for @param) or produce an Identifier expected diagnostic (e.g. on nameless @typedef, callback type-parameter names, function/method declaration names, type-parameter constraint heads). This fixes #4011, where property names with hyphens were emitted unquoted in .d.ts output.
Changes:
- Add
checkNonIdentifierNameto emitIdentifier expectedon invalid identifier names used in reparsed typedef/function/method/type-parameter positions. - Rewrite invalid JSDoc
@paramnames into sanitized identifiers (replacing non-id chars with_, falling back to_<index>), and JSDoc@propertynames into string literals so emit produces quoted property names. - Add a new compiler test
jsdocNonIdentifierPropertiesAndParamsand update affected submodule baselines (mostly adding the newTS1003 Identifier expecteddiagnostic and one converged: error→: anytypes output).
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/parser/reparser.go | Core change: new checkNonIdentifierName/addTransformedReparse helpers and rewriting of param/property names; threads checks through typedef, callback, signature, and type-parameter reparse paths. |
| testdata/tests/cases/compiler/jsdocNonIdentifierPropertiesAndParams.ts | New compiler test exercising hyphenated @property names and a hyphenated @param name in a @callback. |
| testdata/baselines/reference/compiler/jsdocNonIdentifierPropertiesAndParams.{js,types,symbols} | New baselines verifying quoted property names in .d.ts and props_like sanitized parameter name. |
| testdata/baselines/reference/submodule/compiler/misspelledJsDocTypedefTags.* | New diagnostic for nameless typedef trailing junk; type for Request access now resolves to any (closer to TS). |
| testdata/baselines/reference/submodule/compiler/jsdocTypedefNoCrash{,2}.errors.txt(.diff) | New Identifier expected diagnostic on nameless @typedef {{ }}. |
| testdata/baselines/reference/submodule/compiler/jsEnumCrossFileExport.errors.txt(.diff) | Adds extra Identifier expected diagnostic from nameless @typedef {string}. |
| testdata/baselines/reference/submodule/conformance/checkJsdocTypedefOnlySourceFile.errors.txt(.diff) | Same: extra Identifier expected on nameless typedef. |
| testdata/baselines/reference/submodule/conformance/noAssertForUnparseableTypedefs.errors.txt(.diff) | Same diagnostic added at the unparseable typedef site. |
| testdata/baselines/reference/submodule/conformance/typedefInnerNamepaths.errors.txt(.diff) | New Identifier expected at the C~B typedef name. |
| testdata/baselines/reference/submodule/conformance/typedefTagWrapping.errors.txt(.diff) | New Identifier expected for nameless function-typed typedefs across mod1/mod3/mod4. |
Comment on lines
+175
to
+200
| name := jsparam.Name() | ||
| if ast.IsIdentifier(name) && !scanner.IsValidIdentifier(name.AsIdentifier().Text) { | ||
| // drop invalid chars for _, if empty, write _0, etc., so we have a valid param name to emit later | ||
| result := strings.Builder{} | ||
| for i, ch := range name.AsIdentifier().Text { | ||
| if i == 0 { | ||
| if !scanner.IsIdentifierStart(ch) { | ||
| result.WriteRune('_') | ||
| } else { | ||
| result.WriteRune(ch) | ||
| } | ||
| continue | ||
| } else if !scanner.IsIdentifierPart(ch) { | ||
| result.WriteRune('_') | ||
| } else { | ||
| result.WriteRune(ch) | ||
| } | ||
| } | ||
| if result.Len() == 0 { | ||
| result.WriteRune('_') | ||
| result.WriteString(strconv.Itoa(pi)) | ||
| } | ||
| name = p.addTransformedReparse(p.factory.NewIdentifier(result.String()), name) | ||
| } else { | ||
| name = p.addDeepCloneReparse(name) | ||
| } |
Member
|
Can we give a more-specialized error message for reserved names? |
Member
Author
|
@DanielRosenwasser nope, because they're not an error. We're not checking against keywords (which are valid identifiers in many locations), just the actual characters allowed in js identifiers (eg, forbidding a |
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.
Fixes #4011
A lot of the changed baselines are a new
Identifier expectederror on the now unsupported namelesstypedefpattern (which used to pull a name from the next expression, but currently parses an empty string for the name).