Port "Error on override used on dynamically named class members" + use proper JSDoc-related diagnostics
#1043
Conversation
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
…use proper JSDoc-related diagnostics
There was a problem hiding this comment.
Pull Request Overview
This PR ports changes from the referenced TypeScript PR to update error messages regarding misuse of the "override" modifier on dynamically named class members and to require a JSDoc override tag instead in JS files. Key changes include:
- Updating error codes and messages in multiple test baseline files (e.g., from TS4114 to TS4119) for cases of override usage.
- Adjusting the checker logic in internal/checker/checker.go to conditionally report diagnostics based on whether the file is a JS file.
Reviewed Changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| testdata/baselines/reference/submoduleAccepted/conformance/override_js3.errors.txt.diff | Updates error messages from TS4114 to TS4119 for override errors in JS3 context |
| testdata/baselines/reference/submoduleAccepted/conformance/override_js2.errors.txt.diff | Updates error messages from TS4114 to TS4119 in JS2 context |
| testdata/baselines/reference/submoduleAccepted/conformance/jsdocOverrideTag1.errors.txt.diff | Adjusts error messages to require JSDoc comments with the @OverRide tag |
| testdata/baselines/reference/submodule/conformance/override_js3.errors.txt | Updates baseline errors for override errors in the submodule JS3 file |
| testdata/baselines/reference/submodule/conformance/override_js2.errors.txt | Updates baseline errors for override errors in the submodule JS2 file |
| testdata/baselines/reference/submodule/conformance/overrideLateBindableIndexSignature1(...).errors.txt.diff & .txt | Removes outdated errors and adds updated errors for dynamic name scenarios |
| testdata/baselines/reference/submodule/conformance/overrideDynamicName1(...).errors.txt.diff & .txt | Removes outdated errors and adds updated errors for dynamic name scenarios |
| testdata/baselines/reference/submodule/conformance/jsdocOverrideTag1.errors.txt | Updates baseline errors for JSDoc override tag requirements |
| internal/checker/checker.go | Adds conditional logic to select diagnostics based on whether the file is a JS file, differentiating between JSDoc and override modifier errors |
| memberHasOverrideModifier := hasOverrideModifier(member) | ||
| if baseWithThis == nil { | ||
| if memberHasOverrideModifier { | ||
| c.error(member, diagnostics.This_member_cannot_have_an_override_modifier_because_its_containing_class_0_does_not_extend_another_class, c.TypeToString(t)) | ||
| c.error(member, core.IfElse(isJs, diagnostics.This_member_cannot_have_a_JSDoc_comment_with_an_override_tag_because_its_containing_class_0_does_not_extend_another_class, diagnostics.This_member_cannot_have_an_override_modifier_because_its_containing_class_0_does_not_extend_another_class), c.TypeToString(t)) |
There was a problem hiding this comment.
[nitpick] Consider extracting the diagnostic selection logic using core.IfElse into a helper function to reduce duplication and improve maintainability.
Copilot uses AI. Check for mistakes.
Comment on lines
+4473
to
+4484
| c.error(member, core.IfElse(isJs, diagnostics.This_member_cannot_have_a_JSDoc_comment_with_an_override_tag_because_it_is_not_declared_in_the_base_class_0_Did_you_mean_1, diagnostics.This_member_cannot_have_an_override_modifier_because_it_is_not_declared_in_the_base_class_0_Did_you_mean_1), c.TypeToString(baseWithThis), c.symbolToString(suggestion)) | ||
| return | ||
| } | ||
| c.error(member, diagnostics.This_member_cannot_have_an_override_modifier_because_it_is_not_declared_in_the_base_class_0, c.TypeToString(baseWithThis)) | ||
| c.error(member, core.IfElse(isJs, diagnostics.This_member_cannot_have_a_JSDoc_comment_with_an_override_tag_because_it_is_not_declared_in_the_base_class_0, diagnostics.This_member_cannot_have_an_override_modifier_because_it_is_not_declared_in_the_base_class_0), c.TypeToString(baseWithThis)) | ||
| return | ||
| } | ||
| if baseProp != nil && len(baseProp.Declarations) != 0 && !memberHasOverrideModifier && c.compilerOptions.NoImplicitOverride.IsTrue() && node.Flags&ast.NodeFlagsAmbient == 0 { | ||
| baseHasAbstract := core.Some(baseProp.Declarations, hasAbstractModifier) | ||
| if !baseHasAbstract { | ||
| message := core.IfElse(ast.IsParameter(member), | ||
| diagnostics.This_parameter_property_must_have_an_override_modifier_because_it_overrides_a_member_in_base_class_0, | ||
| diagnostics.This_member_must_have_an_override_modifier_because_it_overrides_a_member_in_the_base_class_0) | ||
| core.IfElse(isJs, diagnostics.This_parameter_property_must_have_a_JSDoc_comment_with_an_override_tag_because_it_overrides_a_member_in_the_base_class_0, diagnostics.This_parameter_property_must_have_an_override_modifier_because_it_overrides_a_member_in_base_class_0), | ||
| core.IfElse(isJs, diagnostics.This_member_must_have_a_JSDoc_comment_with_an_override_tag_because_it_overrides_a_member_in_the_base_class_0, diagnostics.This_member_must_have_an_override_modifier_because_it_overrides_a_member_in_the_base_class_0)) |
There was a problem hiding this comment.
[nitpick] Similarly, the diagnostic selection here could be refactored into a common helper to avoid repeating the core.IfElse pattern.
Copilot uses AI. Check for mistakes.
jakebailey
changed the title
Error on override used on dynamically named class members + use proper JSDoc-related diagnosticsError on override used on dynamically named class members + use proper JSDoc-related diagnostics
jakebailey
changed the title
Error on override used on dynamically named class members + use proper JSDoc-related diagnosticsoverride used on dynamically named class members" + use proper JSDoc-related diagnostics
jakebailey
approved these changes
Jun 4, 2025
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.
ports microsoft/TypeScript#60691