-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Bug: [@typescript-eslint/no-unnecessary-type-assertion] False positives for "as const" assertions #8721
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
Comments
This seems like a bug in TS. This is evidenced by the fact that TS types your interface as requiring the property The fact that TS widens the object type to have It's probably worth filing an issue upstream. |
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
Hi @bradzacher, |
Maybe, but as it stands the rule is doing more harm than good by wrongly flagging such |
Hmmmm, note also that -export const marker = "marker" as const;
+export const marker = "marker";
export interface Data {
marker: typeof marker;
value: string;
}
function createData(): Data {
- const base = { marker: marker };
+ const base = { marker: marker } as const;
return { ...base, value: "" };
} works... and makes a whole lot more sense anyway. It seems really odd that using |
In this case, yes, but that will affect all other attributes of the object. For example, if |
-export const marker = "marker" as const;
+export const marker = "marker";
export interface Data {
marker: typeof marker;
value: string;
}
function createData(): Data {
- const base = { marker: marker };
+ const base: { marker: typeof marker } = { marker: marker };
return { ...base, value: "" };
} works also. As does -export const marker = "marker" as const;
+export const marker = "marker";
export interface Data {
marker: typeof marker;
value: string;
}
function createData(): Data {
- const base = { marker: marker };
+ const base = { marker: marker as typeof marker };
return { ...base, value: "" };
} and, of course -export const marker = "marker" as const;
+export const marker = "marker";
export interface Data {
marker: typeof marker;
value: string;
}
function createData(): Data {
const base = { marker: marker };
- return { ...base, value: "" };
+ return { marker, value: "" };
} So it has something to do specifically with the widening behavior when creating a mutable object whose type is inferred. More I think about it, it really just seems surprising to me personally that the example in the issue report works in the first place. |
@kgregory yeah, that's a fair point. and that irked me too. See #8721 (comment) for 3 more examples that don't though 🤣 |
I also note that putting I guess it seems like we're running into a case where a type assertion has broader effect than setting the type of the thing being asserted on.
Curious what other team members think |
I've just learned that TS appears to have intentional behavior around the "freshness" of a In any case, I think it explains what we're seeing here, though I'm not sure that it is obvious what we should actually do about it. @bradzacher, thoughts? |
@kirkwaiblinger I agree with you that we should get clarity on whether this is a TypeScript bug. +1 that it seems odd how having the Marking as blocked on someone filing an issue on TypeScript. |
Posed a question in the TS discord to start 👍 (see https://discord.com/channels/508357248330760243/640177429775777792/1247058925401149482) |
This behaviour is intended in Typescript, to allow constant values to initialise mutable ones: const start = 1001;
const max = 100000;
// many (thousands?) of lines later ...
for (let i = start; i < max; i = i + 1) {
// did I just write a for loop?
// is this a C program?
} (From https://github.com/microsoft/TypeScript/wiki/Reference-Checker-Widening-Narrowing#literal-widening) The best way to write this, in my opinion, is @kirkwaiblinger's second suggestion -- although it would be even better if Typescript allowed Even better, if the program allows, is to use I don't know the specifics of no-unnecessary-type-assertion, but I guess that preferring |
@sandersn Many, many thanks for the excellent explanation and also the helpful reference to the typescript wiki! 🙏 This info also helps us simplify the repro considerably: const freshMarker = 'marker';
let mutableMarker = freshMarker;
const notAssignableDueToWidening: 'marker' = mutableMarker; // TS ERROR - can't assign string to 'marker' vs const unfreshMarker = 'marker' as const; // TYPESCRIPT-ESLINT ERROR - no-unnecessary-type-assertion
let mutableMarker = unfreshMarker;
const assignable: 'marker' = mutableMarker; It's not immediately obvious to me what the best behavior for the rule would be in light of this info. In theory, it might make sense to update the check to be: if the type before and after the assertion are the same and it's not in a context in which freshness could be impacted, consider it an unnecessary type assertion. Seems there's also a good argument that usually the freshness/unfreshness isn't necessary or desired, so from a developer perspective this probably is an unnecessary type assertion in most cases, albeit not one that behaves identically after removing (and therefore which may need to be disabled when the unfresh behavior is genuinely desired). @typescript-eslint/triage-team |
Note also that re-allowing const three = 3 as 3;
// or, maybe selectively allowing this version
const four = 4 as const; would basically amount to relitigating #4485 and #8458 (as noted by @ashvinsrivatsa when pointing out that this presumably changed with #8558) |
👍 that's helpful context, thanks @LukeNotable. I'd be in favor of option 2 or 3, adding an option to the rule. |
So the rule has a pretty good false-positive rate, right? Assuming each error is caused by one false positive - it's correct 92% of the time. So long as we remove the fixer for this case - that would mean that the rule would report and be correct 92% of the time and someone could manually fix it, and the remaining 8% of the time someone could suppress it. 1 still seems like the correct option, IMO |
It would mean all 300 cases are reported by the rule. Removing the fixer would just make it harder if we want to react to that by removing all (or most of) the |
is a terrible false positive rate. I'd suggest simply allowing |
Hey guys, would prefer option 3 - never report it; Here's another example where it makes a difference: With export const defaultLanguage = "en" as const;
const languages = [defaultLanguage];
type DefaultLanguage = (typeof languages)[number];
const lang: DefaultLanguage = "fr"; // Type '"fr"' is not assignable to type '"en"'.ts(2322) Without export const defaultLanguage = "en";
const languages = [defaultLanguage];
type DefaultLanguage = (typeof languages)[number];
const lang: DefaultLanguage = "fr"; // const lang: string Please at least provide an option for advanced TS users to allow |
This comment has been minimized.
This comment has been minimized.
Coming back to this, I'm still in the camp of making the rule lenient by default and adding an opt-in off-by-default option to be more strict. Two reasons why:
We could always turn the option on in Proposal: let's add this option as off-by-default for our current major version (v8), and treat turning it on by default as a separate followup issue? |
+1, I've been swayed based on community feedback and reasons articulated by @JoshuaKGoldberg |
🚀 accepting PRs! |
Just encountered that on production code during eslint / tslint upgrade. Modifying a little bit the playground link there's no easy fix when the Marker type isn't exposed simply (as in a string union deep inside an interface for example) and the rule is even in contradiction with prefer-as-const which is a recommended rule |
… no-unnecessary-type-assertion rule Fixes typescript-eslint#8721
Would it be possible for |
That is a really nifty idea. It would be nice to only add this exemption when it's helpful... But also type inference is a really tricky algorithm to try to re-implement - even just this subset. This feels like a good followup issue & deeper investigation. |
… no-unnecessary-type-assertion rule Fixes typescript-eslint#8721
… no-unnecessary-type-assertion rule Fixes typescript-eslint#8721
… no-unnecessary-type-assertion rule Fixes typescript-eslint#8721
… no-unnecessary-type-assertion rule Fixes typescript-eslint#8721
…nore string const assertions (typescript-eslint#10979) * feat(eslint-plugin): add option to ignore string const assertiions in no-unnecessary-type-assertion rule Fixes typescript-eslint#8721 * review: Enable const assertions on all literals by default * fix: Handle null, undefined and boolean literal expressions * Apply suggestions from code review Co-authored-by: Josh Goldberg ✨ <git@joshuakgoldberg.com> * Apply suggestions from code review typescript-eslint#2
Before You File a Bug Report Please Confirm You Have Done The Following...
Playground Link
https://typescript-eslint.io/play/#ts=5.4.2&fileType=.tsx&code=KYDwDg9gTgLgBAYwgOwM7wLYEMoGthRwC8cARNngaXFqoiugNwCwAUKJLHAJbIwEAzLAmBwAIlhhY4AbzZw4FfFABccGAE8wwCAMU5lLVgoBuWADYBXYGvRReAcyMBfNgMvIEMbikRRgksASUgAUAJRqwdJyxvRo8ABGtKIkMvqUqunKcM5GCv4wllDIsnAAdBVJqMAANHBmVjZk1LlszkA&eslintrc=N4KABGBEBOCuA2BTAzpAXGUEKQAIBcBPABxQGNoBLY-AWhXkoDt8B6Jge1tiacTJTIAhtEK0ipWkOTJE0fJQ5N0UOdA7RI4MAF8QOoA&tsconfig=N4KAvkA&tokens=false
Repro Code
ESLint Config
tsconfig
Expected Result
The first line of this code should not report an error for the
as const
assertion, since this assertion is necessary - removing it is a TypeScript compilation error (see TypeScript playground).Actual Result
Starting in typescript-eslint@7.3 (presumably #8558), the first line of this code reports a @typescript-eslint/no-unnecessary-type-assertion error.
Additional Info
No response
The text was updated successfully, but these errors were encountered: