Fix hover verbosity for nested generic namespace merges#3524
Fix hover verbosity for nested generic namespace merges#3524Andarist wants to merge 1 commit intomicrosoft:mainfrom
Conversation
| if typeArgumentNodes := b.lookupInstantiatedTypeArgumentNodes(chain, index); typeArgumentNodes != nil { | ||
| return typeArgumentNodes | ||
| } else { | ||
| typeParameterNodes := b.typeParametersToTypeParameterDeclarations(symbol) |
There was a problem hiding this comment.
This seems like a departure from Strada, which just called typeParametersToTypeParameterDeclarations in its lookupTypeParameterNodes; why is this change needed?
| return b.f.NewNodeList(typeParameterNodes) | ||
| } | ||
| return nil | ||
| return b.typeParametersToTypeParameterNodes(symbol) |
There was a problem hiding this comment.
I don't think this completely fixes the issue. There are other code paths where we end up with type parameter declarations instead of type nodes (e.g. the branch immediately above this one). In Strada we already dealt with this on printing, since it was a consequence of allowing type arguments attached to identifiers.
There was a problem hiding this comment.
Never mind, I think this does fix everything up. I was confused because lookupInstantiatedTypeArgumentNodes has a confusing return type annotation. It would be nice to use the right type annotation (TypeList) for all of the functions involved here, to indicate intent (until we can hopefully check all this).
There was a problem hiding this comment.
I tested this change in Strada, and the problem is that if we use type nodes here instead of type parameter declarations, we no longer print type parameters with extends clause, e.g. List<T extends Blah> would get printed as List<T> now. So I think we just need to make the printer handle the type parameter declaration case.
There was a problem hiding this comment.
Pull request overview
This PR addresses a hover/QuickInfo crash and incorrect verbosity expansion behavior triggered by nested generic class/namespace merges in the TypeScript-Go language service.
Changes:
- Fix type-parameter/type-argument node construction in
NodeBuilderImpl.lookupTypeParameterNodesto avoid hover verbosity crashes. - Add a focused fourslash test that exercises hover verbosity expansion across a nested generic class/namespace merge.
- Add the corresponding QuickInfo baseline output for the new test.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
internal/checker/nodebuilderimpl.go |
Adjusts how type parameter nodes are produced for qualified-name printing during hover/QuickInfo generation. |
internal/fourslash/tests/hoverVerbosityNestedGenericNamespaceMerge_test.go |
Adds regression coverage for hover verbosity expansion in the problematic nested generic namespace-merge scenario. |
testdata/baselines/reference/fourslash/quickInfo/hoverVerbosityNestedGenericNamespaceMerge.baseline |
Adds expected hover/QuickInfo baseline output for verbosity levels 0–2. |
| targetSymbol := b.ch.getTargetSymbol(symbol) | ||
| if targetSymbol.Flags&(ast.SymbolFlagsClass|ast.SymbolFlagsInterface|ast.SymbolFlagsAlias) != 0 { | ||
| return b.mapToTypeNodes(b.ch.getLocalTypeParametersOfClassOrInterfaceOrTypeAlias(symbol), false /*isBareList*/) | ||
| } else if targetSymbol.Flags&ast.SymbolFlagsFunction != 0 { | ||
| return b.mapToTypeNodes(b.ch.getTypeParametersFromDeclaration(symbol.ValueDeclaration), false /*isBareList*/) | ||
| } |
There was a problem hiding this comment.
In typeParametersToTypeParameterNodes, the branch for functions uses symbol.ValueDeclaration, but you already computed targetSymbol via getTargetSymbol (for instantiated symbols). For instantiated symbols, symbol.ValueDeclaration may be nil, and Checker.getTypeParametersFromDeclaration(nil) will panic. Use targetSymbol (and its ValueDeclaration / declarations) when collecting type parameters to avoid panics and ensure the flags/value come from the uninstantiated symbol.
fixes crash found here: #3512 (comment)