fix(compiler): Expression converter typo - #37361

We definitively need an additional test (one that would fail without the code change and starts passing with the code change)

@pkozlowski-opensource I don't see any unit tests added for the method visitLiteralPrimitive() do I write an integration test

Ideally there should be a test of some kind to demonstrate this.

@AndrewKushnir I tried to write the test but could not understand how to write a test for this can you please help me.

Hi @ajitsinghkaler, I tried to reuse one of the tests from 82bcd83 and was able to trigger the codepath updated in this PR, but I was unable to come up with a case when a test would fail with current state of the master branch and will pass after the change in this PR (which is needed to validate that everything works as needed). May be @petebacondarwin or @alxhub have additional thoughts/ideas. Thank you.

I think the reason we can't make a failing test is that our typing checks tend not to distinguish between null, undefined and false (or even 0!!).

See

@petebacondarwin but the type would be o.INFERRED_TYPE. I think a test that might currently fail would be the following scenario:

<button [id]="computeId(false)"></button>

export class MyCmp {
  doSomething(param: number): string { return ''; }
}

One would expect this not to typecheck, als literal false should be inferred to boolean instead of just any.

Note that since Ivy, any type information on the output AST is not used at all.

I created a template that looks like this:

<div *myIf="person.display != false"> {{person.name}} </div>

where person.display is of type boolean.

The rendered type check block is:

function _View_MainComp_1_0():void {
  var _any:any = (null as any);
  const currVal_0:any = (_decl0_0.person.display != false);
  currVal_0;
}

I believe that because false has been assigned null for its type, it gets cast to a "dynamic" type, which then causes the result of the comparison to also become "dynamic", hence the currVal_0:any.

I'll have a play with a function call as in your suggestion @JoostK .

So a template like:

<button [id]="computeId(false)"></button>

doesn't trigger the visitLiteralPrimitive() method here at all!

After looking into it some more, I can't think of a way to reproduce a bug here.

The type is only relevant if a temporary has to be introduced, which only happens for the LHS of safe navigation operators. Therefore, the literal itself is never assigned into a temporary. In the TS emitter, the type is only used when the literal is null:

For the boolean case, the literal will be emitted as is. Therefore, the literal type is retained.

Alas, I don't think the code with the bug has a real purpose.

@JoostK so should we remove this code

I think that despite no test (and possibly despite this code being used!!) we should land this change since clearly the current code is not correct.

Closing this since it's been several months- feel free to open a new PR that either cleans up the unnecessary code or adds tests for a behavior change.

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}