[6.0.2] Put parentheses around IS NULL for non-bool operands

[roji]

Fixes #26652

Description

EF Core's SQL generation currently omits parentheses around SQL IS (NOT) NULL, in some cases where these are required.

Customer impact

When using database providers other than SQL Server, the LINQ expression Where(b => b.SomeBool = b.SomeNonBoolColumn.HasValue) returns incorrect results.

How found

Customer reported (originally in PomeloFoundation/Pomelo.EntityFrameworkCore.MySql#1309)

Regression

No

Testing

Added test for this scenario.

Risk

Very low, the fix simply adds parentheses around all occurrences of IS (NOT) NULL.

[roji]

This would add lots of parentheses in various places, will hold off until @smitpatel takes a look, who's looking into parentheses questions elsewhere as well.

[smitpatel]

@roji - I am not sure if I have a solution for this in 6.0.x. My fix requires adding new API and providers overriding it which is 7.0 work. How did this work in previous release? May be we can just mimic it.

[Isitar]

@smitpatel this worked before 6.x with the following code in npgsql:
npgsql/efcore.pg@313840d

[smitpatel]

Sqlite has IS NULL as same priority as comparison but lower than arithmetic.
SqlServer doesn't really specify it explicitly but same as comparison.
Postgre has higher priority than comparison.

Given left-to-right behavior for same priority operations, we need to put brackets around IS NULL if it is being used during comparison operation. @roji - You should be able to put some hack in VisitSqlBinary method to make it work in patch release.

[roji]

@smitpatel yeah, this isn't an urgent issue for me in Npgsql - I can bring back the hack I already had up to now.

But re Sqlite:

Sqlite has IS NULL as same priority as comparison but lower than arithmetic.

Right, but:

SELECT TRUE <> (NULL IS NULL); -- returns 0
SELECT TRUE <> NULL IS NULL; -- return 1

... because since the precedence is the same, SQLite evaluates left to right, giving incorrect results...

[roji]

Changed this to apply the same hack as for PostgreSQL - the advantage is that this is SQLite-only and does not change SQL Server's SQL. Rebased this on release/6.0 to consider for patching - the main branch should instead have the proper solution @smitpatel is working on (but keep the added test when merging this).

[roji]

Yeah, @smitpatel I think this is only waiting for a review.

[wtgodbe]

@roji looks like tactics didn't review this, if you want to get it in for February could you send mail to netcoretac before EOD?

[roji]

Sent. If this is approved by tactics, could you (or someone else from the team) please merge it as it's quite late here in Europe?

[wtgodbe]

Approved over email

[roji]

Thanks @wtgodbe