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

Flag JS Literals and ignore assignments/accesses to invalid props, instead of adding an index #25996

Merged
merged 9 commits into from Aug 2, 2018

Conversation

weswigham
Copy link
Member

@weswigham weswigham commented Jul 27, 2018

Fixes #25987

As mentioned in-person with @mhegazy, since our goal with the index signature was to just squelch a class of errors in JS, this accomplishes the same thing without the unfortunate side effect of mangling our inferences. This has the fortunate side-effect that the meaningless index signatures are no longer present in quickinfo and the like.

These accesses are also now marked in noImplicitAny mode now - which I'd argue is a correct thing to do. That is the breaking change referenced in the PR labels, however in practice it's unlikely to break much (and when it does it's probably desirable!) as JS codebases using noImplicitAny are relatively rare.

@brendankenny
Copy link
Contributor

As an aside, these accesses could totally be marked in noImplicitAny mode now - which I'd argue is a correct thing to do; we can put off deciding to do that, though.

One downside of this change (if I'm understanding it correctly) is that not having [x: string]: any; in the interface anymore means that it's going to be more difficult to diagnose what's going on when those accesses are not marked as an error. Right now it's kind of a process of hovering for quickinfo and then realizing, "oh yeah, tsc treats object literals loosely here" :)

Having keyof, etc work as expected is definitely an improvement for checked js, but it would be fantastic if we're also able to opt into errors for them with noImplicitAny.

@sandersn
Copy link
Member

You may have told me in person, but: how do the user tests do with this change?

@mhegazy
Copy link
Contributor

mhegazy commented Jul 30, 2018

Since we are touching this area, i agree with @brendankenny that we should make --noImplicitAny work as expected. we have talked about this before but never done it.

@weswigham weswigham force-pushed the alter-js-affordances-strategy branch from 19e24a1 to 8d41b63 Compare August 1, 2018 00:31
@weswigham
Copy link
Member Author

@mhegazy Alright, we'll no longer squeltch the errors under noImplicitAny. It's up now~

@sandersn I fixed a pair of small problems (like also disabling excess property checks on js literals outside of noImplicitAny mode), but it looks like now the changes are mostly pretty minimal. There's a lot of churn of index signatures getting removed and I believe errors from the recent lib update which we'll need to look over tomorrow; but this looks pretty good, IMO.

@weswigham weswigham added the Breaking Change Would introduce errors in existing code label Aug 1, 2018
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, just some nits about comments.

// * unions where every element is a jsliteral
// * intersections where at least one element is a jsliteral
// * and instantiable types constrained to a jsliteral
// Should all count as literals and not print errors on access or assignment of imaginary properties.
Copy link
Member

Choose a reason for hiding this comment

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

quibble: these properties are not "imaginary". They are "possibly existing" properties.

Seriously, though, I'd say "expando properties". I also don't think that referencing the old implementation helps, so I'd delete that line or at least change it to "This mirrors the behaviour of index signatures".

@@ -9072,6 +9075,34 @@ namespace ts {
return type;
}

/**
* Returns if a type is or consists of a JSLiteral object type
Copy link
Member

Choose a reason for hiding this comment

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

I would move the comment block up here.

@weswigham
Copy link
Member Author

I'm going to hold off on merging this until our user/rwc baselines are clean, so we can clearly see its impact on them. AFAIK we're not shipping this in a 3.0.2 (it's technically a break, after all), so this isn't in much of a rush.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking Change Would introduce errors in existing code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants