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

extended diagnostic for non-nullable optional is wrong #46918

Open
tiberiuzuld opened this issue Jul 21, 2022 · 34 comments
Open

extended diagnostic for non-nullable optional is wrong #46918

tiberiuzuld opened this issue Jul 21, 2022 · 34 comments
Assignees
Labels
area: compiler Issues related to `ngc`, Angular's template compiler bug compiler: extended diagnostics P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent
Milestone

Comments

@tiberiuzuld
Copy link

tiberiuzuld commented Jul 21, 2022

Which @angular/* package(s) are the source of the bug?

core

Is this a regression?

No

Description

Having the case below produces the warning for optional chain operation.
Because of ! it will always return a boolean to the input.
Relates to the new feature #46686
#44870

Also can we have a angularCompilerOptions to disable this warnings?

First case:

optionalArrayElement: {a: 'value'}[] = [];
<button [disabled]="!optionalArrayElement[0]?.a"></button>

Second case:

enum MapKeys {
  A,
  B
}

 optionalMapElement: {[key: string]: {a: string}} = {[MapKeys.A] : {a: 'value'}};
<button [disabled]="!optionalMapElement[MapKeys.B]?.a"></button>

Please provide a link to a minimal reproduction of the bug

https://stackblitz.com/edit/angular-r3n3ec?file=src/app/app.component.ts

Please provide the exception or error you saw

warning NG8107: The left side of this optional chain operation does not include 'null' or 'undefined' in its type, therefore the '?.' operator can be replaced with the '.' operator.

Please provide the environment you discovered this bug in (run ng version)

Angular CLI: 14.1.0
Node: 16.15.1
Package Manager: npm 8.13.2
OS: win32 x64

Angular: 14.1.0
... animations, cli, common, compiler, compiler-cli, core, forms
... platform-browser, platform-browser-dynamic, router
... service-worker

Package                         Version
---------------------------------------------------------
@angular-devkit/architect       0.1401.0
@angular-devkit/build-angular   14.1.0
@angular-devkit/core            14.1.0
@angular-devkit/schematics      14.1.0
@schematics/angular             14.1.0
rxjs                            7.5.6
typescript                      4.7.4

Anything else?

No response

@pkozlowski-opensource pkozlowski-opensource added area: compiler Issues related to `ngc`, Angular's template compiler compiler: extended diagnostics labels Jul 21, 2022
@ngbot ngbot bot added this to the needsTriage milestone Jul 21, 2022
@pkozlowski-opensource
Copy link
Member

@tiberiuzuld could you please share more details / reproduction scenario? We were briefly looking into it but the types and assigned values don't match thus making the problem confusing. Your type is written as any[] | null but the value being assigned ({b: null}) doesn't match this type. Did you mean [{b: null}]?

In short - the type, assigned value and expression don't seem to match / make sense together so I'm assuming there is some typo / missing info here.

@alxhub alxhub added the needs reproduction This issue needs a reproduction in order for the team to investigate further label Jul 21, 2022
@ngbot ngbot bot modified the milestones: needsTriage, Backlog Jul 21, 2022
@tiberiuzuld
Copy link
Author

Hello @pkozlowski-opensource ,
Sorry yes, I missed typed the type of a in the description.
I updated the initial description. I hope is more clear now.

@pkozlowski-opensource pkozlowski-opensource removed the needs reproduction This issue needs a reproduction in order for the team to investigate further label Jul 21, 2022
@ngbot ngbot bot modified the milestones: Backlog, needsTriage Jul 21, 2022
@pkozlowski-opensource
Copy link
Member

Thnx for the update @tiberiuzuld - yes indeed, it looks like a valid bug report, thnx!

@pkozlowski-opensource pkozlowski-opensource added the P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent label Jul 21, 2022
@ngbot ngbot bot modified the milestones: needsTriage, Backlog Jul 21, 2022
@tiberiuzuld
Copy link
Author

tiberiuzuld commented Jul 21, 2022

Hmm I was investigating the issue a bit some more.
It seems our types are wrong since how I did in the stackblitz.
The warning is being displayed only if we put the ? and the type of b is only as array (updated description).

Don't know if you want to include this as a bug.

@tiberiuzuld
Copy link
Author

I found another case where I think the bug is valid.
Second case with optionalArrayElement see stackblitz example in the main description.

@pkozlowski-opensource
Copy link
Member

OK, so the first case is definitively not a bug and works as expected. The second optionalArrayElement: {a: 'value'}[] = []; case is more interesting.

@pkozlowski-opensource
Copy link
Member

@tiberiuzuld could you please update the issue description to remove references to the first case (as this works as intended?)

@tiberiuzuld
Copy link
Author

tiberiuzuld commented Jul 21, 2022

@pkozlowski-opensource
Ok I will, also found same issue when you have a map optionalMapElement updated stackblitz.
Will update the description also.

@tiberiuzuld
Copy link
Author

tiberiuzuld commented Jul 21, 2022

Workaround possible with making a getter/method in the class ts:

get firstElement():  {a: 'value'} | undefined {
  return this.optionalArrayElement[0];
}

getElement(index: number):  {a: 'value'} | undefined {
  return this.optionalArrayElement[index];
}
<button [disabled]="firstElement?.a"></button>
<button [disabled]="getElement(0)?.a"></button>

@tiberiuzuld
Copy link
Author

image

@sod
Copy link
Contributor

sod commented Jul 22, 2022

we had a similar issue, see #44859 (comment) - when array[1234] ?? 'fallback' threw a warning about supposedly array[1234] never being null

@BenRacicot
Copy link

I'd also like to turn off this warning.

@nunoarruda
Copy link
Contributor

I also think that this NG8107 warning is broken. For example:

  1. I have an optional templateForm property in my TS class of type UntypedFormGroup so its inferred types are UntypedFormGroup | undefined

Screenshot 2022-07-26 at 13 27 37

  1. Yet, Angular says that The left side of this optional chain operation does not include 'null' or 'undefined' in its type, therefore the '?.' operator can be replaced with the '.' operator. which is incorrect since an optional property can possibly be undefined.

Screenshot 2022-07-26 at 13 30 13

@JoostK
Copy link
Member

JoostK commented Jul 26, 2022

I also think that this NG8107 warning is broken. For example:

  1. I have an optional templateForm property in my TS class of type UntypedFormGroup so its inferred types are UntypedFormGroup | undefined
Screenshot 2022-07-26 at 13 27 37
  1. Yet, Angular says that The left side of this optional chain operation does not include 'null' or 'undefined' in its type, therefore the '?.' operator can be replaced with the '.' operator. which is incorrect since an optional property can possibly be undefined.
Screenshot 2022-07-26 at 13 30 13

@nunoarruda can you please share a runnable reproduction?


As for the indexed access false positive: this happens because TypeScript doesn't include undefined in the type of an indexed access expression (foo[index] syntax), so the optional chaining operator appears unnecessary based on the type system. I think it makes sense not to report this diagnostic for indexed accesses.

@nunoarruda
Copy link
Contributor

nunoarruda commented Jul 26, 2022

@JoostK it looks like it is working fine in an isolated demo:
Screenshot 2022-07-26 at 16 23 59
Now the template is correctly assuming the type of the property as UntypedFormGroup | undefined. So I'm not sure what's going on with my project but I will let you folks know if I find anything along the way.

@danielehrhardt
Copy link

danielehrhardt commented Jul 28, 2022

I'd also like to turn off this warning.

"angularCompilerOptions": {
    "strictNullChecks": false
  }

@JoostK
Copy link
Member

JoostK commented Jul 29, 2022

I'd also like to turn off this warning.


"angularCompilerOptions": {

    "strictNullChecks": false

  }

Please do not do this. strictNullChecks is not an Angular compiler option, and disabling the TypeScript option is definitely not recommended.

This particular rule can be disabled like any other extended diagnostic, as documented here: https://angular.io/extended-diagnostics#configuration. The rule name is optionalChainNotNullable, which is currently not yet included in the docs.

@danielehrhardt
Copy link

optionalChainNotNullable does not work for me

@dcbroad3
Copy link

optionalChainNotNullable does not work for me

Can verify that this did fix it for me. Seems to require at least angular 14.1.

@robleka
Copy link

robleka commented Aug 3, 2022

Hello. Got the same problem all over my application where things can clearly be null or undefined but I receive the warning regardless. Two examples bellow:

s01
s02
s03
s04

Angular CLI: 14.1.0
Node: 16.13.1
Package Manager: npm 8.1.2
OS: win32 x64

Angular: 14.1.0
... animations, cdk, cli, common, compiler, compiler-cli, core
... forms, material, material-moment-adapter, platform-browser
... platform-browser-dynamic, router

Package Version

@angular-devkit/architect 0.1401.0
@angular-devkit/build-angular 14.1.0
@angular-devkit/core 14.1.0
@angular-devkit/schematics 14.1.0
@angular/flex-layout 14.0.0-beta.40
@schematics/angular 14.1.0
rxjs 7.5.6
typescript 4.7.4

@anisabboud
Copy link

@robleka the first case you mentioned is because you defined person!: (the ! is the non-null assertion operator).
Replace the exclamation mark with a question mark (person?:) and the errors should go away.

(Regarding the second case, I think it's because item is defined as CommonTypes.Any, which I'm guessing maps to any.)

@robleka
Copy link

robleka commented Aug 3, 2022

Thanks, that fixed the first issue. But the second one is still strange to me as "item" is optional. Yes, it can be of any type, but it still is optional, it might not exist or it may be null.

@nunoarruda
Copy link
Contributor

nunoarruda commented Aug 3, 2022

Tip: watch out for optional properties inside *ngIf blocks. If you're checking if an optional property is truthy then references of that property inside that *ngIf block will always be truthy and thus the ?. operator is unnecessary. Example:

TS:
user?: {firstName: string, lastName: string};

Template:

<div *ngIf="user">
    <div>{{user?.firstName}}</div> <!-- unnecessary -->
    <div>{{user.firstName}}</div> <!-- ok -->
</div>

@ghovmand
Copy link

ghovmand commented Aug 4, 2022

Got the same warning:

Warning: src/main/webapp/skattekoersel/koersel/components/beregning/beregning-noegletal/beregning-noegletal.component.html:87:23 - warning NG8107: The left side of this optional chain operation does not include 'null' or 'undefined' in its type, therefore the '?.' operator can be replaced with the '.' operator.

87           {{ koersel?.noegletal?.antalKanIkkeFordeles | number }}
                         ~~~~~~~~~

After I "fixed" the warning it, got following error:

Error: src/main/webapp/skattekoersel/koersel/components/beregning/beregning-noegletal/beregning-noegletal.component.html:87:33 - error TS2532: Object is possibly 'undefined'.

87           {{ koersel?.noegletal.antalKanIkkeFordeles | number }}
                                   ~~~~~~~~~~~~~~~~~~~~

@anisabboud
Copy link

@ghovmand try koersel.noegletal?.antalKanIkkeFordeles

@JoostK
Copy link
Member

JoostK commented Aug 4, 2022

It appears there is some confusion which optional chain is redundant. It's always the one to the left of the underlined property. If I recall correctly it wasn't possible to highlight just the operator, hence we now highlight the property name, but perhaps this can be improved as well. I'm currently on vacation though, so won't be able to look into this at the moment.

@ghovmand
Copy link

ghovmand commented Aug 4, 2022

Thanks, that solved the issue for me; always look to the left :)

@e-oz
Copy link

e-oz commented Aug 10, 2022

An example of how to suppress this check right now (it's not obvious from previous comments):

  "angularCompilerOptions": {
    "extendedDiagnostics": {
      "checks": {
        "optionalChainNotNullable": "suppress"
      }
    }
  }

@Alex-Sessler
Copy link

Alex-Sessler commented Aug 10, 2022

I am seeing this warning quite a lot with this pattern:

class A {
    b: MyType
    
    constructor(private activatedRoute: ActivatedRoute) {
         b = activatedRoute.snapshot.data.b
    }
}

Which is quite common, as i tend to fetch the data in a resolver and assign it to the variable in the constructor.
Now when i do a conditional check in a template like this:

<div *ngIf="b?.c"></div>
It shows me the warning. I feel like angular should be able to detect that me declaring the variable without an initializer implies that i can actually be undefined and not force me to bloat my code by declaring my variable like this:

b: MyType | undefined

I definitely don't want to disable these checks, as they are definitely useful. But in this case it feels like angular should handle this case internally.

EDIT:
I feel like the same should count for @ViewChild properties, as these are by definition undefined until after the template was initialized. These should also by default be recognized as possibly undefined.

@dcbroad3
Copy link

dcbroad3 commented Aug 10, 2022

I am seeing this warning quite a lot with this pattern:

class A {
    b: MyType
    
    constructor(private activatedRoute: ActivatedRoute) {
         b = activatedRoute.snapshot.data.b
    }
}

This is effectively exactly the same as:

class A {
  b: MyType = this.activatedRoute.snapshot.data.b;
  
  constructor(private activatedRoute: ActivatedRoute) { }
}

In other words, in this class b is undefined only if the data from ActivatedRoute is undefined. In strict mode TypeScript will actually tell you, you need to add undefined to the type if the property is not assigned directly where it is declared or in the constructor.

There's no way to automatically mark things like @ViewChild as possibly undefined, that should be done manually and strict mode will require you to declare them as possibly undefined.

image

@kmsayem12
Copy link

"extendedDiagnostics": {
      "checks": {
        "optionalChainNotNullable": "suppress"
      }
    }

@e-oz Thank you work for me on the angular 14 version

JoostK added a commit to JoostK/angular that referenced this issue Aug 22, 2022
…cesses

TypeScript's type system does not include `undefined` in the type of an indexed
access, whereas array index accesses may be expected to be out-of-bounds.
This commit updates the extended diagnostic for unnecessary optional chains to
ignore indexed accesses.

Closes angular#46918
@JoostK JoostK self-assigned this Aug 22, 2022
@JamesLuRepo
Copy link

I meet the same bugThe left side of this optional chain operation does not include 'null' or 'undefined' in its type, therefore the '?.' operator can be replaced with the '.' operator
when I run the code
<img src="{{ product?.imageUrl }}" class="detail-img">
where I would like to use it to avoid race condition

One of the possible solutions is to replace the product?.imageUr with product&&product.imageUrl.
Thus,
<img src="{{ product&&product.imageUrl }}" class="detail-img">

@josueixcot
Copy link

@roblekael primer caso que mencionó se debe a que definió person!:( !es el operador de aserción no nulo ). Reemplace el signo de exclamación con un signo de interrogación ( person?:) y los errores deberían desaparecer.

(Con respecto al segundo caso, creo que es porque el elemento se define como CommonTypes.Any, que supongo que se asigna a any).

this worked for me, thanks!

@JIBIN-P
Copy link

JIBIN-P commented Jan 22, 2024

The same error occurred to me, have suppressed this warning for now by using #46918 (comment)
I hope this gets a fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: compiler Issues related to `ngc`, Angular's template compiler bug compiler: extended diagnostics P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent
Projects
None yet
Development

Successfully merging a pull request may close this issue.