Skip to content

TS: allow namespace imports as types#1138

Merged
semmle-qlci merged 4 commits intogithub:rc/1.20from
asger-semmle:ts-import-namespace-as-type
Mar 21, 2019
Merged

TS: allow namespace imports as types#1138
semmle-qlci merged 4 commits intogithub:rc/1.20from
asger-semmle:ts-import-namespace-as-type

Conversation

@asger-semmle
Copy link
Contributor

Apparently I was very careful to not allow namespaces to be seen as types, but apparently now they can. Fixes some FPs reported by our unused import query.

@asger-semmle asger-semmle added this to the 1.20 milestone Mar 20, 2019
@asger-semmle asger-semmle requested a review from a team as a code owner March 20, 2019 10:14
Copy link

@xiemaisi xiemaisi left a comment

Choose a reason for hiding this comment

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

Basically LGTM. Slightly adventurous as a hotfix, but I trust you have thought about the benefits and risks involved.

locations_default(#20002,#10000,1,1,1,1)
hasLocation(#20001,#20002)
#20003=*
jsParseErrors(#20003,#20001,"Error: Unexpected token"," yield import(""foo"")

Choose a reason for hiding this comment

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

This doesn't look right; that syntax error shouldn't be there on the rc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sigh. That was a case of Eclipse needing a refresh

@ghost
Copy link

ghost commented Mar 20, 2019

Can you check if it solves this https://jira.semmle.com/browse/ODASA-7792 issue as well?

@asger-semmle
Copy link
Contributor Author

Can you check if it solves this https://jira.semmle.com/browse/ODASA-7792 issue as well?

That was fixed previosly. I've closed the issue.

xiemaisi
xiemaisi previously approved these changes Mar 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants