Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stop reassigning .valueDeclaration to avoid replacing earlier declarations with late ones #60857

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Andarist
Copy link
Contributor

@Andarist Andarist commented Dec 26, 2024

cc @sandersn

fixes #60590

@typescript-bot typescript-bot added the For Backlog Bug PRs that fix a backlog bug label Dec 26, 2024
@Andarist Andarist changed the title Don't merge late bound assignment members with pre-existing symbols Stop reassigning .valueDeclaration to avoid replacing earlier declarations with late ones Dec 26, 2024
@@ -13197,10 +13197,8 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
else if (!member.symbol.isReplaceableByMethod) {
symbol.declarations.push(member);
}
if (symbolFlags & SymbolFlags.Value) {
if (!symbol.valueDeclaration || symbol.valueDeclaration.kind !== member.kind) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was the original logic trying to preserve the somewhat-sketchy logic in the binder around which type of declaration "wins" out?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It sure looks like it, but there's a separate utility function for that now: setValueDeclaration, which is more complicated*. Specifically, the kind check is only for namespaces there, which wouldn't apply to this property assignments.

I think the better fix to try is

if (symbolFlags & SymbolFlags.Value) {
  setValueDeclaration(symbol, member)
}

In other words, what the binder does in addDeclarationToSymbol.

*It also has a 3rd case for function property assignments in .d.ts files.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pushed out this change. Could you take another look?

src/compiler/checker.ts Outdated Show resolved Hide resolved
@@ -13197,10 +13197,8 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
else if (!member.symbol.isReplaceableByMethod) {
symbol.declarations.push(member);
}
if (symbolFlags & SymbolFlags.Value) {
if (!symbol.valueDeclaration || symbol.valueDeclaration.kind !== member.kind) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It sure looks like it, but there's a separate utility function for that now: setValueDeclaration, which is more complicated*. Specifically, the kind check is only for namespaces there, which wouldn't apply to this property assignments.

I think the better fix to try is

if (symbolFlags & SymbolFlags.Value) {
  setValueDeclaration(symbol, member)
}

In other words, what the binder does in addDeclarationToSymbol.

*It also has a 3rd case for function property assignments in .d.ts files.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Backlog Bug PRs that fix a backlog bug
Projects
Status: Waiting on author
Development

Successfully merging this pull request may close these issues.

JSDoc property modifiers on computed properties
4 participants