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.

Sign up for GitHub

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

Jump to bottom

Make nonnull assertions and binding patterns apparent declared type locations #20995

Merged
merged 3 commits into from
Jan 20, 2018
Merged

Make nonnull assertions and binding patterns apparent declared type locations #20995

merged 3 commits into from
Jan 20, 2018

Conversation

weswigham
Copy link
Member

Fixes #20994

@ahejlsberg
Copy link
Member

Please explain what the issue is and how this fix solves it.

@weswigham
Copy link
Member Author

In the process of doing control flow analysis for a, we replace it with it's filtered constraints in either branch of the conditional following the destructuring pattern, then union them back together. In this way, T["a"] becomes undefined | number | object. We fire off a used before defined check because we don't see that undefined is in the domain of T["a"]. By checking it's apparent type for falsy flags, we check against the constraint instead, which yields the correct result.

@Jessidhia
Copy link

Jessidhia commented Jan 4, 2018 •
edited
Loading

This seems to fix the problem, but I don't think it's fixing the actual problem, which is the "use before assignment" assertion on something that has been assigned. Regardless of what the type of T["a"] is, after let { a } = anything, a is definitely assigned. If T["a"]'s domain includes undefined, that just makes so that a's type can include undefined.

(NOTE: I'm not very familiar with how the checking works internally, so I'm commenting based on the explanation above)

@weswigham
Copy link
Member Author

@Kovensky the way that we do use before assignment checks in strict null checks mode is all about the type. You can only use before assignment a type without undefined in it's domain, so if the type at a specific location still has undefined in it (which is introduced by a declaration without an assignment) but didn't have it in the declared type, then it must still be there because it hasn't been assigned yet. At least, that's the assumption we currently make to issue the error.

@mhegazy
Copy link
Contributor

mhegazy commented Jan 8, 2018

LGTM. @ahejlsberg any comments?

@weswigham
Copy link
Member Author

@ahejlsberg This now uses the fix we talked about in person.

@weswigham weswigham changed the title Use apparent type of original type to handle truthiness of indexes Make nonnull assertions and binding patterns apparent declared type locations Jan 18, 2018
@sandersn sandersn mentioned this pull request Jan 19, 2018
Closed
@mhegazy mhegazy mentioned this pull request Jan 19, 2018
Closed
ahejlsberg
ahejlsberg requested changes Jan 19, 2018
View reviewed changes
Copy link
Member

@ahejlsberg ahejlsberg 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 once we have a better name for that function.

src/compiler/checker.ts Outdated
}

function typeHasNullableConstraint(type: Type) {
return type.flags & TypeFlags.TypeVariable && maybeTypeOfKind(getBaseConstraintOfType(type) || emptyObjectType, TypeFlags.Nullable);
}

function getDeclaredOrApparentType(symbol: Symbol, node: Node) {
function getDeclaredOrApparentType(type: Type, node: Node) {
Copy link
Member

Choose a reason for hiding this comment

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

We need a better name for this method.

Copy link
Member Author

@weswigham weswigham Jan 19, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Renamed to getApparentTypeForLocation

@weswigham weswigham force-pushed the uninitialized-oddity branch from a062dc9 to 5dfffb1 Compare January 19, 2018 21:52
@weswigham
Copy link
Member Author

@ahejlsberg done

@weswigham weswigham merged commit d4c3612 into microsoft:master Jan 20, 2018
@weswigham weswigham deleted the uninitialized-oddity branch January 20, 2018 00:06
@ajafff ajafff mentioned this pull request Jan 20, 2018
Closed
@ajafff
Copy link
Contributor

ajafff commented Jan 20, 2018

@weswigham I think @Kovensky is right that fixing the issue mentioned above could be achieved with a smaller change.
Identifiers whose declaration is a BindingElement are always initialized. That means you could just add another condition to the initializer of assumeInitialized inside checkIdentifier (currently line 13043).

errendir added a commit to errendir/TypeScript that referenced this pull request Jan 20, 2018
@errendir
Merge remote-tracking branch 'origin/master' into faster-interface-check
bff2f37 
* origin/master: (134 commits)
  Fix isTypeOfExpression in compiler API (microsoft#20875). (microsoft#20884)
  add completion filter for function like body (microsoft#21257)
  Make nonnull assertions and binding patterns apparent declared type locations (microsoft#20995)
  For `{ type: "a" } | { type: "b" }`, find references for the union property (microsoft#21298)
  configureNightly -> configurePrerelease
  Create a 'configure-insiders' and 'publish-insiders' task.
  Add createProgram on WatchCompilerHost
  in goToDefinition, use array helpers and clean up code (microsoft#21304)
  Support testing definition range of a reference gruop (microsoft#21302)
  Handle `undefined` input to firstDefined (microsoft#21300)
  Avoid spreading array (microsoft#21291)
  LEGO: check in for master to temporary branch.
  Accept new baselines
  Add regression test
  Properly handle contravariant inferences in inferReverseMappedType
  Remove unused properties from interface Refactor (microsoft#21286)
  LEGO: check in for master to temporary branch.
  Fold newline logic into getNewLineOrDefaultFromHost
  External test runner updates (microsoft#21276)
  Report more detailed info during script debug failure
  ...
@weswigham
Copy link
Member Author

@ajafff This also undoes an older bugfix which caused the issue in the first place, and instead fixes that bug differently. That's why the change is large.

@ajafff ajafff mentioned this pull request Jan 23, 2018
Closed
@microsoft microsoft locked and limited conversation to collaborators Jul 3, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ahejlsberg ahejlsberg ahejlsberg requested changes

@DanielRosenwasser DanielRosenwasser Awaiting requested review from DanielRosenwasser

@mhegazy mhegazy Awaiting requested review from mhegazy

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@weswigham @ahejlsberg @Jessidhia @mhegazy @ajafff