Skip to content

Conversation

asger-semmle
Copy link
Contributor

Fixes a problem observed when extracting DefinitelyTyped in full mode.

@asger-semmle asger-semmle requested a review from a team as a code owner March 7, 2019 18:45
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

LGTM.

  • do we want this as a hotfix for 1.20?
  • does it require a change note?

* Returns `true` if the properties of the given type can safely be extracted
* without restricting expansion depth.
*
* This predicate is very approximate, and considers all unions, intersections,
Copy link

Choose a reason for hiding this comment

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

Very approximate indeed.
Maybe we should look into special casing on some common cases later.

Copy link
Contributor Author

@asger-semmle asger-semmle Mar 8, 2019

Choose a reason for hiding this comment

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

It shouldn't be that bad. The structure of a type is still extracted, this just determines whether its properties are. So for a type like Buffer | string, we can still tell it's a Buffer | string, but we won't necessarily compute the members of the union itself (i.e. the intersection of members on Buffer and string).

Also, members are always computed for types directly referenced from the AST, for example, if some expression is inferred to have the type A & B, that type is fully extracted, members and all, but A and B themselves might be shallow.

(edited for a better example)

@xiemaisi xiemaisi added the JS label Mar 8, 2019
@asger-semmle asger-semmle force-pushed the ts-infinite-expansion branch from ca1dbee to 16a2177 Compare March 11, 2019 11:37
@asger-semmle asger-semmle requested a review from a team as a code owner March 11, 2019 11:37
@asger-semmle asger-semmle changed the base branch from master to rc/1.20 March 11, 2019 11:37
@asger-semmle asger-semmle removed the request for review from a team March 11, 2019 11:37
@asger-semmle asger-semmle added this to the 1.20 milestone Mar 11, 2019
@asger-semmle
Copy link
Contributor Author

Rebased on 1.20. I'm not sure a change note is warranted, given that not many people use full-mode extraction anyway.

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.

LGTM; let's skip the change note.

@semmle-qlci semmle-qlci merged commit a2b1939 into github:rc/1.20 Mar 11, 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