Skip to content

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

Closed
4 tasks done
ashvinsrivatsa opened this issue Mar 19, 2024 · 33 comments · Fixed by #10979
Closed
4 tasks done
Labels
accepting prs Go ahead, send a pull request that resolves this issue bug Something isn't working locked due to age Please open a new issue if you'd like to say more. See https://typescript-eslint.io/contributing. package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin

Comments

@ashvinsrivatsa
Copy link

Before You File a Bug Report Please Confirm You Have Done The Following...

  • I have tried restarting my IDE and the issue persists.
  • I have updated to the latest version of the packages.
  • I have searched for related issues and found none that matched my issue.
  • I have read the FAQ and my problem is not listed.

Playground Link

https://typescript-eslint.io/play/#ts=5.4.2&fileType=.tsx&code=KYDwDg9gTgLgBAYwgOwM7wLYEMoGthRwC8cARNngaXFqoiugNwCwAUKJLHAJbIwEAzLAmBwAIlhhY4AbzZw4FfFABccGAE8wwCAMU5lLVgoBuWADYBXYGvRReAcyMBfNgMvIEMbikRRgksASUgAUAJRqwdJyxvRo8ABGtKIkMvqUqunKcM5GCv4wllDIsnAAdBVJqMAANHBmVjZk1LlszkA&eslintrc=N4KABGBEBOCuA2BTAzpAXGUEKQAIBcBPABxQGNoBLY-AWhXkoDt8B6Jge1tiacTJTIAhtEK0ipWkOTJE0fJQ5N0UOdA7RI4MAF8QOoA&tsconfig=N4KAvkA&tokens=false

Repro Code

export const marker = "marker" as const;
export interface Data {
  marker: typeof marker;
  value: string;
}
function createData(): Data {
  const base = { marker: marker };
  return { ...base, value: "" };
}

ESLint Config

module.exports = {
  "rules": {
    "@typescript-eslint/no-unnecessary-type-assertion": "error"
  }
}

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

@ashvinsrivatsa ashvinsrivatsa added bug Something isn't working package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for team members to take a look labels Mar 19, 2024
@bradzacher
Copy link
Member

bradzacher commented Mar 19, 2024

This seems like a bug in TS.
Without the as const the type of marker is "marker". With it the type is the same.

This is evidenced by the fact that TS types your interface as requiring the property marker: "marker", not marker: string.

The fact that TS widens the object type to have marker: string unless the variable declaration is specifically marked with as const seems like counter-intuitive and incorrect behaviour.

It's probably worth filing an issue upstream.

@bradzacher bradzacher added awaiting response Issues waiting for a reply from the OP or another party and removed triage Waiting for team members to take a look labels Mar 19, 2024
@klassm

This comment was marked as off-topic.

@bradzacher

This comment was marked as off-topic.

@JulienBara-360
Copy link

Hi @bradzacher,
I met the same problem today.
If it helps, rolling back from v7.4.0 to v6.21.0 removed the false positive.
In the meantime, I kept typescript on v5.4.3.

@LukeNotable
Copy link

The fact that TS widens the object type to have marker: string unless the variable declaration is specifically marked with as const seems like counter-intuitive and incorrect behaviour.

It's probably worth filing an issue upstream.

Maybe, but as it stands the rule is doing more harm than good by wrongly flagging such as const as safely removable.

@kgregory
Copy link

Ran into this as well.

It appears to have been introduced with 7.3.0, possibly with #8558 - Does not occur after rolling back to 7.2.0.

@kirkwaiblinger
Copy link
Member

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 as const in the first line would have an effect on the type of base granted marker has the same type whether or not as const is explicitly used.

@kgregory
Copy link

kgregory commented May 10, 2024

works... and makes a whole lot more sense anyway.

In this case, yes, but that will affect all other attributes of the object. For example, if base were of a type with an array attribute, applying as const to base would type that array as readonly. (see this playground)

@kirkwaiblinger
Copy link
Member

-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.

@kirkwaiblinger
Copy link
Member

works... and makes a whole lot more sense anyway.

In this case, yes, but that will affect all other attributes of the object. For example, if base were of a type with an array attribute, applying as const to base would type that array as readonly. (see this playground)

@kgregory yeah, that's a fair point. and that irked me too. See #8721 (comment) for 3 more examples that don't though 🤣

@kirkwaiblinger
Copy link
Member

kirkwaiblinger commented May 10, 2024

I also note that putting const marker = 'marker' as 'marker' also has the same impact in these examples as const marker = 'marker' as const.

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

@kirkwaiblinger kirkwaiblinger added triage Waiting for team members to take a look and removed awaiting response Issues waiting for a reply from the OP or another party labels May 10, 2024
@kirkwaiblinger
Copy link
Member

kirkwaiblinger commented May 17, 2024

I've just learned that TS appears to have intentional behavior around the "freshness" of a const variable type (see microsoft/TypeScript#52473 (comment)), though it's not clear that it's a particularly external-facing concept. Would have to do some more research.

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?

@JoshuaKGoldberg
Copy link
Member

@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 as const on line 1 doesn't change the reported type of the const marker, but does change the type of the const base = { marker: marker }.

Marking as blocked on someone filing an issue on TypeScript.

@JoshuaKGoldberg JoshuaKGoldberg added blocked by another issue Issues which are not ready because another issue needs to be resolved first and removed triage Waiting for team members to take a look labels Jun 3, 2024
@kirkwaiblinger
Copy link
Member

kirkwaiblinger commented Jun 3, 2024

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)

@sandersn
Copy link
Contributor

sandersn commented Jun 3, 2024

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 as const on object properties.

Even better, if the program allows, is to use "marker" everywhere instead of extracting to a const variable. The compiler checks "marker" everywhere Data is used from here on out, so there shouldn't be a problem with typos which I guess is one reason for extracting to a variable.

I don't know the specifics of no-unnecessary-type-assertion, but I guess that preferring as const on mutable locations instead of immutable ones means that it's currently correct and doesn't need to change.

@kirkwaiblinger
Copy link
Member

kirkwaiblinger commented Jun 5, 2024

@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

@kirkwaiblinger
Copy link
Member

kirkwaiblinger commented Jun 5, 2024

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)

@kirkwaiblinger kirkwaiblinger added triage Waiting for team members to take a look and removed blocked by another issue Issues which are not ready because another issue needs to be resolved first labels Jun 5, 2024
@JoshuaKGoldberg
Copy link
Member

👍 that's helpful context, thanks @LukeNotable. I'd be in favor of option 2 or 3, adding an option to the rule.

@bradzacher
Copy link
Member

300 scattered cases...
25 TypeScript errors...

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

@LukeNotable
Copy link

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.

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 as const. However, we probably don't, because (1) it would lead to inconsistency where some cases have as const (with an eslint-disable) and some don't, just to satisfy the linter, and (2) it partly undermines what the as const is there for in the first place. That is, the rule is only "correct" in many cases in the sense that it doesn't currently result in TypeScript errors, but it's still wrong in the sense that the as const is not wholly superfluous.

@dmurvihill
Copy link

the rule would report and be correct 92% of the time

is a terrible false positive rate.

I'd suggest simply allowing as const, if applied to a literal that can be widened.

@Maxim-Mazurok
Copy link
Contributor

Maxim-Mazurok commented Oct 29, 2024

Hey guys, would prefer option 3 - never report it;
Since it does make a difference in TypeScript, and is a pretty popular feature of the language (even though more on the advanced side).

Here's another example where it makes a difference:

With as const:

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 as const:

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 as const case. Of course it would be awesome if we could auto-detect type widening possibilities, but probably isn't worth the effort on linter side.

This comment has been minimized.

@kirkwaiblinger kirkwaiblinger added triage Waiting for team members to take a look and removed accepting prs Go ahead, send a pull request that resolves this issue labels Nov 1, 2024
@JoshuaKGoldberg
Copy link
Member

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:

  • The fact that TypeScript's behavior is so mysterious as to make us need to check in with the team hints to me that this will be a big confusing pain point if we enable it by default
  • The as const practice in question is a common use case that people need - especially as evidenced by the big pushback here.

We could always turn the option on in strictTypeChecked - though IMO that should be a separate followup issue, to not block this one.

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?

@kirkwaiblinger
Copy link
Member

+1, I've been swayed based on community feedback and reasons articulated by @JoshuaKGoldberg

@JoshuaKGoldberg JoshuaKGoldberg added accepting prs Go ahead, send a pull request that resolves this issue and removed triage Waiting for team members to take a look labels Nov 10, 2024
@JoshuaKGoldberg
Copy link
Member

🚀 accepting PRs!

@KuSh
Copy link
Contributor

KuSh commented Mar 21, 2025

Just encountered that on production code during eslint / tslint upgrade.
The autofix completely killed my 100k LOC project and unfortunately isn't disableable atm

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

@controversial
Copy link
Contributor

Would it be possible for typescript-eslint to detect whether or not a constant variable is later used as part of type inference for another variable (i.e. a let or an object declaration) in order to decide whether or not an as const assertion is unnecessary?

@JoshuaKGoldberg
Copy link
Member

Would it be possible for typescript-eslint to detect whether or not a constant variable is later used as part of type inference for another variable (i.e. a let or an object declaration) in order to decide whether or not an as const assertion is unnecessary?

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.

KuSh added a commit to KuSh/typescript-eslint that referenced this issue Mar 24, 2025
KuSh added a commit to KuSh/typescript-eslint that referenced this issue Apr 14, 2025
KuSh added a commit to KuSh/typescript-eslint that referenced this issue Apr 18, 2025
KuSh added a commit to KuSh/typescript-eslint that referenced this issue Apr 18, 2025
phaux pushed a commit to phaux/typescript-eslint that referenced this issue Apr 24, 2025
…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
@github-actions github-actions bot added the locked due to age Please open a new issue if you'd like to say more. See https://typescript-eslint.io/contributing. label Apr 29, 2025
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 29, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepting prs Go ahead, send a pull request that resolves this issue bug Something isn't working locked due to age Please open a new issue if you'd like to say more. See https://typescript-eslint.io/contributing. package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin
Projects
None yet