Hi @miguel-leon !

Could you explain why new.target would be exempted?

From a glance, I noted:

• the expression new.target has type function Fn(): void, which is always truthy, so the rule is behaving as expected based on the TS types.
• As long as the noImplicitAny option is enabled (or strict), it is illegal TS to construct new Fn(), so, it didn't seem like well-formed TS code should really be able to hit the case that new.target is undefined anyway

WDYT?

Hi @kirkwaiblinger,

why new.target would be exempted?

From the docs:

...functions invoked using the new operator, new.target returns a reference to the constructor or function that new was called upon. In normal function calls, new.target is undefined.

the expression new.target has type function Fn(): void

Looks like an oversight on typescript's part not to have it be a union with undefined, at least if(new.target) is not a TS error.

As long as the noImplicitAny option is enabled (or strict), it is illegal TS to construct new Fn()

Are you saying that all projects that use the linter must have those options?
If so, would you consider this not well-formed TS code?
If the argumentation is that under no circumstance one should use new.target or new with functions; well, that would be a subjective assessment and I won't be able to argue it.

This seems like a TypeScript bug. It should not allow code like this:

function Fn() {
  console.log(new.target.name);
}

microsoft/TypeScript#48344 is related, but I didn't find anything about undefined. Perhaps you should file an issue? In any case, I don't think we should special case this in our rule; fix TS and the rule automatically fixes itself.

Are you saying that all projects that use the linter must have those options?

Not exactly, but it informs our willingness to special-case things that can already be handled natively by TS... noImplicitAny is widely recommended and widely used, so edge cases that only arise in non-noImplicitAny code are unlikely to be deemed worth special-casing.

If so, would you consider this not well-formed TS code? If the argumentation is that under no circumstance one should use new.target or new with functions; well, that would be a subjective assessment and I won't be able to argue it.

Heh, I'll admit that I've never seen code like that and I don't have an opinion on it yet 😅.

In any case, on a re-read I agree with your and Josh's observation that the TS type appears to be incorrect, so addressing that is the best way to proceed.

Since that TS bug is causing typescript-eslint's rule to error in a place not expected by the user, and you consider typescript-eslint needs not to account for this; would you consider the issue in TS be filed by a typescript-eslint representative? plus it'll surely have more weight than if I did it.

noImplicitAny is widely recommended and widely used

I'm pretty sure in a future "major" release they'll be changing the defaults for the compiler options to make things better by default.

so edge cases that only arise in non-noImplicitAny code are unlikely to be deemed worth special-casing.

We don't seek to cover all cases with rules. There are bound to be false positives in some edge cases that we don't or cannot handle.

This rule is pretty straightforward in its design - it is powered by types. If the types are wrong then the rule is wrong - simple as that.

The only special casing it has is around records and arrays where TS internally doesn't model the undefined properly.

If so, would you consider this not well-formed TS code?

Yes - I personally would not consider it well-formed code.

In modern JS and TS having functions that may be both called as a function or called as a constructor is widely considered unnecessarily permissive.

Most people seek to reduce API surface area and reduce variance in how one consumes things to reduce choice and complexity where it doesn't add value.

There's no reason to not just be one or the other either in TS given that TS can easily downlevel class syntax if you're running in an env that doesn't support classes.

@miguel-leon

Since that TS bug is causing typescript-eslint's rule to error in a place not expected by the user, and you consider typescript-eslint needs not to account for this; would you consider the issue in TS be filed by a typescript-eslint representative? plus it'll surely have more weight than if I did it.

I'd recommend you give it a try! The TS folks are pretty egalitarian 🙂. I'd recommend linking this issue as relevant context, though, of course!

@bradzacher

I appreciate your opinion, though you either contradicted yourself, or I didn't understand what you meant, or something else.

Most people seek to reduce API surface area and reduce variance in how one consumes things to reduce choice and complexity where it doesn't add value.

If there are two functionalities, I'd understand as less API surface having a single identifier rather than two. And if the functionalities are usable only in distinct places (for example as a decorator and as a base class), then there's no variance increase either.

@kirkwaiblinger

I was trying to reason about it and return the suggestion. I know they're egalitarian and I've reported plenty there. I feel it's not up to me to do it in this case, so I won't do it.

Even if the reason is external to this package; as a user, typescript-eslint is the package that is producing me an unexpected error; so I reported it here. To me, it seems reasonable that this package either escalates the bug to its dependency, or disables the rule for the unreliable case new.target.

If there are two functionalities, I'd understand as less API surface having a single identifier rather than two.

Well it depends really. If your intent is that Foo() and new Foo() provide entirely different outputs -- then sure that is "two functionalities". Though I would argue that using the same identifier doesn't reduce API surface here -- there's the same API surface, after all even if it a singular identifier.

Though if that's your intent I'd further lean into this being not-well-formed TS code. Making a function work as a class or as a function historically was done in JS as a way to be forgiving to people -- to allow people to "forget new" and still have the API work. But the design was always that Foo() and new Foo() would return the same thing -- a new instance of the Foo class. If your intent is that they aren't returning the same thing then you're diverging from established practices, hence "not-well-formed" code.

As explained -- this rule is powered by the types in the code.
If the types are wrong then the types need to be fixed.

We suggest that users raise an issue with the owners of the types to drive improvement and change there.

We, the maintainers of typescript-eslint, are volunteers and so our time + bandwidth is limited. Getting these types corrected is not something that is important to us as we do not use new.target in any of our work, and the majority of our users ¹ do not use the feature either. As such we shall not raise an issue with the owners of the types.

If this is important to you, please raise an issue with the owners of the types to drive the change to correct the types so that they correctly reflect reality.

Well, having Foo() and new Foo() return the same thing would look like a smell to me because one has an operator an the other doesn't. In any case, I wouldn't let Foo be used differently only by that, because the two expressions are too similar.

Also, I think the definition for "not-well-formed" has no relation to "established practices", so you might need to look that up. Ultimately, all your negative connotations about certain "ways to code" are simply personal opinion and debatable. I would be more humble about them, but I appreciate them nonetheless.

Getting these types corrected is not something that is important to us as we do not use new.target in any of our work

To be clear, I don't really care if the issue gets fixed or doesn't. At the end of the day, this is your project, you decide who is allowed to add changes, and you decide how complete and accurate you want the project to be.

As I stated earlier, I report this here because, for the reasons I explained, this is the appropriate place to do so. Afterall, typescript-eslint is the one erroring on something that doesn't "reflect reality".

Do with this issue what you will. Specially if the fix takes too much time. Although I'd argue that writing all these justifications would take longer.