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

[Master] Fix resolve entity name to not dive inside property access expression when the expression is not entity name #14692

Merged
merged 3 commits into from
Mar 29, 2017

Conversation

yuit
Copy link
Contributor

@yuit yuit commented Mar 16, 2017

While writing tests for dynamic import, I ran into Debug.fail("Unknown entity name kind."); inside resolveEntityName function, turned out that it not only causes failure when writing out ".types" files but also disable some language services features (see test files)

@yuit yuit mentioned this pull request Mar 21, 2017
5 tasks
@yuit yuit merged commit 0cb490e into master Mar 29, 2017
@yuit yuit deleted the master-fixResolveEntityName branch March 29, 2017 16:55
Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

Looks good, although the new fallback behaviour in getSymbolOfEntityNameOrPropertyAccess is somewhat confusing.

//// }
//// var x: {
//// B : CTor
//// };
Copy link
Member

Choose a reason for hiding this comment

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

lines 1-9 don't seem necessary for this test

//// }
//// var x: {
//// B : CTor
//// };
Copy link
Member

Choose a reason for hiding this comment

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

lines 1-9 don't seem necessary for this test

//// }
//// var x: {
//// B : CTor
//// };
Copy link
Member

Choose a reason for hiding this comment

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

lines 1-9 don't seem necessary for this test

//// class C extends (foo()).[|B|] {}
//// class C1 extends foo().[|B|] {}

const [def, ref1, ref2] = test.ranges();
Copy link
Member

Choose a reason for hiding this comment

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

Try this instead:

const rs = test.ranges();
for (const r of rs) {
  verify.referencesOf(r, rs));
}

@@ -1511,6 +1525,15 @@ namespace ts {
return undefined;
}
}
else if (name.kind === SyntaxKind.ParenthesizedExpression) {
// If the expression in parenthsizedExpression is not an entity-name (e.g. it is a call expression), it won't be able to successfully resolve the name.
Copy link
Member

Choose a reason for hiding this comment

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

typo:parenthesized

// i.e class C extends foo()./*do language service operation here*/B {}
return isEntityNameExpression(name.expression) ?
resolveEntityName(name.expression as EntityNameOrEntityNameExpression, meaning, ignoreErrors, dontResolveAlias, location) :
undefined;
Copy link
Member

Choose a reason for hiding this comment

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

should we also give an error in the undefined case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We shouldn't because the caller will already handle this case.

}
var x: {
B : CTor
};
Copy link
Member

Choose a reason for hiding this comment

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

turns out lines 1-7 aren't used here either!

@microsoft microsoft locked and limited conversation to collaborators Jun 21, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants