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

New user experience: type/variable confusion #14450

Closed
stephencelis opened this issue Mar 4, 2017 · 14 comments
Closed

New user experience: type/variable confusion #14450

stephencelis opened this issue Mar 4, 2017 · 14 comments
Labels
Question An issue which isn't directly actionable in code

Comments

@stephencelis
Copy link

stephencelis commented Mar 4, 2017

TypeScript Version: 2.1.1

Variable/type name confusion makes debugging difficult with default configuration.

Code

const add1 = (x: number) => (y: number): number => x + y

add1("no")("no")

Leads to the expected error:

[ts] Argument of type '"no"' is not assignable to type 'number'.

My experience with other type systems led me to try this when abstracting the function type signature into a type alias:

type add2 = (number) => (number) => number
const add2: add2 = x => y => x + y

add2("no")("no") // no error?

This, I would find out, is because the above is type is actually the following signature:

(number: any) => (number: any) => number

Expected behavior:

This was a bit confusing, coming from other languages where variable names are usually meaningless in type signatures. I'd hope for a gentle warning with a configuration option to disable. This might make the experience a bit more forgiving to folks new to TypeScript.

It turns out that setting "noImplicitAny" to true easily flags these situations, but that option is opt-in.

Actual behavior:

No warning.

@stephencelis
Copy link
Author

stephencelis commented Mar 4, 2017

This was a matter of confusion in the end.

type Button = ({ title: string }) => JSX.Element
const Button: Button = ({ title }) => <button>{title}</button>

<Button title={42} />

The above doesn't error out, though I imagined it should. The correct typealias ended up being this:

type Button = (props: { title: string }) => JSX.Element

Any chance we can get some kind of error/notice on the previous type declaration? Or am I missing the case in which it's valid?

@stephencelis stephencelis reopened this Mar 4, 2017
@gcanti
Copy link

gcanti commented Mar 4, 2017

type add2 = (number) => (number) => number

is equivalent to

type add2 = (number: any) => (number: any) => number

Note that setting "noImplicitAny": true helps in both your examples

@stephencelis
Copy link
Author

@gcanti Thanks! The errors that option supplies makes it much easier to remember about accidental type shadowing. I do wonder if there's a world in-between where the default warns when you shadow type names by variable name—fewer gotchas for newcomers.

@stephencelis stephencelis changed the title type-aliasing suppresses type errors. New user experience: type/variable confusion Mar 4, 2017
@stephencelis
Copy link
Author

I've made some edits above to clarify where I believe a confusing situation for new users could be made more friendly.

@ikatyang
Copy link
Contributor

ikatyang commented Mar 4, 2017

tslint rule from tslint-microsoft-contrib has already supported this feature.

Rule Name Description
no-reserved-keywords Do not use reserved keywords as names of local variables, fields, functions, or other identifiers. Since version 2.0.9 this rule accepts a parameter called allow-quoted-properties. If true, interface properties in quotes will be ignored. This can be a useful way to avoid verbose suppress-warning comments for generated d.ts files.
type add2 = (number) => (number) => number // warning
const add2: add2 = x => y => x + y

add2("no")("no")

[tslint] Forbidden reference to reserved keyword: number (no-reserved-keywords)

It will be a good experience for new user, although I think it is the role of linter.

@RyanCavanaugh RyanCavanaugh added the Question An issue which isn't directly actionable in code label Mar 6, 2017
@RyanCavanaugh
Copy link
Member

See #13152, #3081.

We'd prefer if you used Stack Overflow for getting-started questions, or searched more to find existing issues. Needless to say after 14,000 bugs, simple stuff like this has already been discussed.

@stephencelis
Copy link
Author

stephencelis commented Mar 6, 2017

@RyanCavanaugh Fair enough, though doesn't the fact that this keeps coming up make you consider it an open user experience issue?

@RyanCavanaugh
Copy link
Member

RyanCavanaugh commented Mar 6, 2017

noImplicitAny already exists and we can't possibly change the behavior at this point due to backward compatibility. Users encountering a stumbling block does not imply an "open" issue.

@RyanCavanaugh
Copy link
Member

What I mean is, if you have a concrete suggestion, we're definitely willing to hear it, but we've thought about this a lot and haven't come up with a satisfactory solution, and thus consider this "closed" in the sense of "There are no good options on the table despite substantial effort, and we don't think more effort will produce new options".

@stephencelis
Copy link
Author

stephencelis commented Mar 6, 2017

My concrete suggestion from the OP:

I'd hope for a gentle warning [on by default] with a configuration option to disable. This might make the experience a bit more forgiving to folks new to TypeScript.

It turns out that setting "noImplicitAny" to true easily flags these situations, but that option is opt-in.

@stephencelis
Copy link
Author

(If I'm not being clear enough, this would be a new configuration option that lives alongside noImplicitAny.)

@RyanCavanaugh
Copy link
Member

That'd be a breaking change similar to turning on noImplicitAny by default.

@stephencelis
Copy link
Author

Could you elaborate? A new compiler warning with helpful instructions on how to disable seems like a fair tradeoff. I'm happy to drop this, it's just definitely a pain point for new user on-boarding, and I've been looking to the helpful experiences that Elm and Swift (sometimes) provide. I imagine there'd be an interest in making it as painless as possible to introduce new users of all experiences to TypeScript.

@RyanCavanaugh
Copy link
Member

RyanCavanaugh commented Mar 6, 2017

There is definitely code in the real world which uses name-only function signatures on purpose. Our general rule is that we only introduce new warnings when we strongly believe those warnings are "correct" (i.e. we found a bug where we failed to find a bug before). Anything else is behind an opt-in flag.

So since you'd have to opt in to this flag anyway, the question is, who turns on this flag but doesn't want to turn on noImplicitAny ? The difference between the two is quite small and it's really unclear why you would turn on noImplicitAnyParameters (or whatever you'd call it) instead of noImplicitAny.

@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Question An issue which isn't directly actionable in code
Projects
None yet
Development

No branches or pull requests

4 participants