fix: correct name length criterion for spelling fixes

[AlCalzone]

Using Math.min effectively invalidated the name length criterion and prevented some otherwise good candidates from being suggested.

Fixes #49557

[ghost]

All CLA requirements met.

[AlCalzone]

Compiler tests are failing with baselines that seem unrelated to what I did here. I suppose I can't just accept them, right?

[DanielRosenwasser]

Can you accept the rest of the baselines?

[AlCalzone]

@DanielRosenwasser Done. Most of the changes are good IMO, except the one I highlighted. I mean the suggested name is similar, but the previous error message was better. Maybe the criteria need some tuning now?

[jakebailey]

Hm, this is also unfortunate; it's effectively never that someone wants to use Function. Maybe this is rare enough?

[AlCalzone]

This happens for prefixes with length 1-4, so aFunction, myFunction, ..., someFunction but not longer prefixes - and for things like aPhunction.

[AlCalzone]

but not this. Maybe the check for TS2301 should not happen in arrow function assignments?

[jakebailey]

I don't think x should be in scope for this arrow function; x in this example is just a constructor parameter.

I'm a little confused as to why all of these say that it's "declared in the constructor"; my brain is telling me that it's something that's assigned to this in the constructor, when it's just some variable that happens to be in scope for the constructor function, so it's a little weird. I would think that "cannot find name x" is technically the more accurate message, because there's nothing you can do to x in the constructor signature (not even private x: number) that would make it accessible. (But, there is that comment that says "it's legal in this one situation", which is complicating.)

I guess I'd have to go back to when this diagnostic was added to see what the intent was.

[sandersn]

The reason for the error is that pre-ES2021 (2022?), TS defaults to emitting class properties as assignments in the constructor (controllable separately from target with useDefinedForClassFields), so private a = x would emit:

class A {
  constructor(x) {
    this.a = x
  }
}

Then there are two problems:

1. The thing you wrote should not find name 'x' -- so you'd expect the old error.
2. But it will at runtime, because the emitted code in the constructor -- so you'd expect the new error.

The new error message is correct when useDefineForClassFields: false, which it is for this file, and also extremely obscure.

Note that you can get the new error without the old one if you add var x = 1 at the beginning of the test. The new error is possibly even more confusing in this case.

So: the new error is correct and also horrible. I don't know if we should fix it (separately) though: the top hit for the error is a pretty good explanation from 2015. And over time fewer people will target non-compliant emit.

Minutes later: The error still shows up when useDefineForClassFields: true, which it shouldn't. I'll file a bug for that.

[sandersn]

Filed #49719

[AlCalzone]

And this is not an initializer 🤔

[jakebailey]

It's just the message that was wrong; I guess this message didn't come up a lot so nobody realized the wording didn't make sense if the error location was inside a type node. It's an easy fix; just make another message that says "Type" instead. I'm pushing that now.

[jakebailey]

Or, we could just say "instance member variable 'b' cannot reference identifier 'x' declared in the constructor."

That would be agnostic and remove a bit of logic I had to add to distinguish the cases, but not necessarily point to any one part (but, the error range will be there, so it might be fine).

[jakebailey]

Looks a lot better now, but I am a little concerned about the initializer messages.

[jakebailey]

I don't think x should be in scope for this arrow function; x in this example is just a constructor parameter.

I'm a little confused as to why all of these say that it's "declared in the constructor"; my brain is telling me that it's something that's assigned to this in the constructor, when it's just some variable that happens to be in scope for the constructor function, so it's a little weird. I would think that "cannot find name x" is technically the more accurate message, because there's nothing you can do to x in the constructor signature (not even private x: number) that would make it accessible. (But, there is that comment that says "it's legal in this one situation", which is complicating.)

I guess I'd have to go back to when this diagnostic was added to see what the intent was.

[AlCalzone]

Most of the concerns are addressed with the latest commit. I'm not sure if this https://github.com/microsoft/TypeScript/pull/49575/files/7effd54c0224207b142997ad98e9456eeb5be776#r908149168 is desired or not, but I'm also not sure how to fix it.

I won't be available for the next 2 weeks, so if anyone from the team wants to pick up and finish this, feel free to.

[jakebailey]

I won't be available for the next 2 weeks, so if anyone from the team wants to pick up and finish this, feel free to.

I just updated your branch to be up to date with main and with baselines accepted. I'll quick look into the message you're noting; everything else seems okay to me at this point.

[jakebailey]

I don't really like what I've done here; I just want to know "is errorLocation in the type of the property declaration". I was going to use findAncestor, but it needs to stop once the property declaration node is reached, so that makes it a little more than a one liner, and just checking that the node contains the other node by position is fast. This is done in one other place, so it seemed okay.