Avoid constructing empty extends clauses in hovers#3507
Avoid constructing empty extends clauses in hovers#3507DanielRosenwasser wants to merge 7 commits intomainfrom
extends clauses in hovers#3507Conversation
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
There was a problem hiding this comment.
Pull request overview
Fixes a hover/quick info crash in the TypeScript-Go language server when expanding class declarations whose base type cannot be represented as a valid extends heritage expression (e.g., mixin constructors returning intersections).
Changes:
- Skip emitting an
extendsheritage clause when no validExpressionWithTypeArgumentsnodes can be constructed for the base types. - Add a new fourslash test covering hover verbosity on a class extending a mixin constructor that returns an intersection type.
- Add the corresponding reference baseline for the new fourslash test.
Show a summary per file
| File | Description |
|---|---|
internal/checker/nodebuilder_hover.go |
Filters out nil heritage expressions and avoids constructing an empty extends clause during hover expansion. |
internal/fourslash/tests/quickinfoVerbosityClassWithMixinBase_test.go |
New regression test exercising hover verbosity expansion for a mixin base returning an intersection. |
testdata/baselines/reference/fourslash/quickInfo/quickinfoVerbosityClassWithMixinBase1.baseline |
New expected hover output baseline for the added test. |
Copilot's findings
- Files reviewed: 3/3 changed files
- Comments generated: 1
| defer testutil.RecoverAndFail(t, "Panic on fourslash test") | ||
|
|
||
| // Expanded hover serializes Derived's generated class declaration, | ||
| // including the class heritage clause for a constructor returning an intersection. |
There was a problem hiding this comment.
The comment says the expanded hover includes the class heritage clause, but the expected baseline output for verbosity level 1 shows class Derived {} with no extends .... Please update the comment to describe the actual intended behavior (e.g. that we omit an extends clause when we can't construct any heritage expressions) so the test remains self-explanatory.
| // including the class heritage clause for a constructor returning an intersection. | |
| // omitting an extends clause when no heritage expression can be constructed | |
| // from a constructor type returning an intersection. |
gabritto
left a comment
There was a problem hiding this comment.
In light of the recent issues we've had in this same code path, I'm wondering if maybe the right thing to do here is to just print the heritage clauses as they exist in the declaration(s). I think we're not supposed to expand those, so I'm not sure anything is gained by reconstructing the heritage clauses from the base types, which potentially loses information that the declaration had...
| // ^^^^^^^ | ||
| // | ---------------------------------------------------------------------- | ||
| // | ```typescript | ||
| // | class Derived |
There was a problem hiding this comment.
Could we somehow just print extends Mixin here?
Co-authored-by: Copilot <copilot@github.com>
Fixes #3506