Skip to content

Linq: Adds support constant evaluation of Nullable<T>.HasValue#3273

Merged
ealsur merged 11 commits intoAzure:masterfrom
ccurrens:fix-constant-eval
Jul 12, 2022
Merged

Linq: Adds support constant evaluation of Nullable<T>.HasValue#3273
ealsur merged 11 commits intoAzure:masterfrom
ccurrens:fix-constant-eval

Conversation

@ccurrens
Copy link
Copy Markdown
Contributor

@ccurrens ccurrens commented Jun 16, 2022

Description

This commit adds support for evaluating Nullable<T>.HasValue when the original nullable constant's HasValue is false.

Nullable<T> is tricky to use with reflection because of it's special runtime behavior. This manually determines if HasValue was false, and in that case returns a constant expression of false.

More details are in #3272.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Closing issues

closes #3272

@ealsur
Copy link
Copy Markdown
Member

ealsur commented Jun 16, 2022

/azp run

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines Bot commented Jun 16, 2022

Azure Pipelines successfully started running 2 pipeline(s).

@ealsur
Copy link
Copy Markdown
Member

ealsur commented Jun 16, 2022

@khdang Please take a look

FabianMeiswinkel
FabianMeiswinkel previously approved these changes Jun 21, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@FabianMeiswinkel FabianMeiswinkel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - Thanks!

@ealsur ealsur added the LINQ label Jun 21, 2022
@ealsur ealsur changed the title Linq: support constant evaluation of Nullable<T>.HasValue Linq: Adds support constant evaluation of Nullable<T>.HasValue Jun 21, 2022
@ealsur
Copy link
Copy Markdown
Member

ealsur commented Jul 7, 2022

/azp run

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines Bot commented Jul 7, 2022

Azure Pipelines successfully started running 2 pipeline(s).

@leminh98 leminh98 self-requested a review July 7, 2022 17:14
leminh98
leminh98 previously approved these changes Jul 7, 2022
View reviewed changes
@ealsur ealsur requested a review from adityasa as a code owner July 7, 2022 17:23
@ealsur
Copy link
Copy Markdown
Member

ealsur commented Jul 7, 2022

/azp run

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines Bot commented Jul 7, 2022

Azure Pipelines successfully started running 2 pipeline(s).

@ealsur ealsur enabled auto-merge (squash) July 7, 2022 17:40
@ealsur
Copy link
Copy Markdown
Member

ealsur commented Jul 7, 2022

/azp run

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines Bot commented Jul 7, 2022

Azure Pipelines successfully started running 2 pipeline(s).

khdang
khdang reviewed Jul 8, 2022
View reviewed changes
Comment thread Microsoft.Azure.Cosmos/src/Linq/SubtreeEvaluator.cs Outdated
@ealsur ealsur enabled auto-merge (squash) July 11, 2022 17:22
@ealsur
Copy link
Copy Markdown
Member

ealsur commented Jul 11, 2022

/azp run

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines Bot commented Jul 11, 2022

Azure Pipelines successfully started running 2 pipeline(s).

khdang
khdang previously approved these changes Jul 11, 2022
View reviewed changes
@ealsur
Copy link
Copy Markdown
Member

ealsur commented Jul 11, 2022

@ccurrens It looks like the last commit is failing on the LinqTranslationBaselineTests.TestMemberAccess I'll try rerunning everything, but the error is:

Expected: ><![CDATA[SELECTVALUErootFROMrootWHERE(root["NumericField"]=4)]]></SqlQuery></Output></Result><Resul,
Actual:   ><![CDATA[]]></SqlQuery><ErrorMessage><![CDATA[Objectreferencenotsettoaninstanceofanobject.]]></Erro,
OutputPath: BaselineTest\TestOutput\LinqTranslationBaselineTests.TestMemberAccess.xml,
BaselinePath: BaselineTest\TestBaseline\LinqTranslationBaselineTests.TestMemberAccess.xml

@ealsur
Copy link
Copy Markdown
Member

ealsur commented Jul 11, 2022

/azp run

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines Bot commented Jul 11, 2022

Azure Pipelines successfully started running 2 pipeline(s).

@ccurrens
Copy link
Copy Markdown
Contributor Author

ccurrens commented Jul 12, 2022 •
edited
Loading

@ccurrens It looks like the last commit is failing on the LinqTranslationBaselineTests.TestMemberAccess I'll try rerunning everything, but the error is:

Expected: ><![CDATA[SELECTVALUErootFROMrootWHERE(root["NumericField"]=4)]]></SqlQuery></Output></Result><Resul,
Actual:   ><![CDATA[]]></SqlQuery><ErrorMessage><![CDATA[Objectreferencenotsettoaninstanceofanobject.]]></Erro,
OutputPath: BaselineTest\TestOutput\LinqTranslationBaselineTests.TestMemberAccess.xml,
BaselinePath: BaselineTest\TestBaseline\LinqTranslationBaselineTests.TestMemberAccess.xml

Thanks for the heads up. I messed up the conversion of the first pattern expression back to what it needed to be. I'll post up the fix in just a minute.

targetConstant is { Value: null } was doing a null check on targetConstant and on targetConstant.Value. I only did the latter.

auto-merge was automatically disabled July 12, 2022 01:08

Head branch was pushed to by a user without write access

@ccurrens ccurrens dismissed stale reviews from khdang and ealsur via d01484c July 12, 2022 01:08
@ccurrens
Copy link
Copy Markdown
Contributor Author

ccurrens commented Jul 12, 2022

The property pattern for targetConstant is { Value: null } and the new check targetConstant != null && targetConstant.Value == null produce the same assembly (under x64 ryuJit at least), but I've leave it as discrete checks instead of the pattern match. Seemed safer since I don't know about other architectures.

@ealsur
Copy link
Copy Markdown
Member

ealsur commented Jul 12, 2022

/azp run

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines Bot commented Jul 12, 2022

Azure Pipelines successfully started running 2 pipeline(s).

@ealsur
Copy link
Copy Markdown
Member

ealsur commented Jul 12, 2022

/azp run

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines Bot commented Jul 12, 2022

Azure Pipelines successfully started running 2 pipeline(s).

@ealsur
Copy link
Copy Markdown
Member

ealsur commented Jul 12, 2022

/azp run

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines Bot commented Jul 12, 2022

Azure Pipelines successfully started running 2 pipeline(s).

@ealsur ealsur enabled auto-merge (squash) July 12, 2022 16:25
@ealsur ealsur merged commit be8814f into Azure:master Jul 12, 2022
@xinlian12 xinlian12 mentioned this pull request Mar 18, 2026
Merged
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@khdang khdang khdang approved these changes

@ealsur ealsur ealsur left review comments

@FabianMeiswinkel FabianMeiswinkel FabianMeiswinkel left review comments

@leminh98 leminh98 leminh98 left review comments

@sboshra sboshra Awaiting requested review from sboshra sboshra is a code owner

@neildsh neildsh Awaiting requested review from neildsh neildsh is a code owner

@kirankumarkolli kirankumarkolli Awaiting requested review from kirankumarkolli kirankumarkolli is a code owner

@j82w j82w Awaiting requested review from j82w

@kirillg kirillg Awaiting requested review from kirillg kirillg is a code owner

@adityasa adityasa Awaiting requested review from adityasa adityasa is a code owner

Assignees

No one assigned

Labels

LINQ

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Constant evaluation of Nullable<T>.HasValue fails with System.Reflection.TargetException: Non-static method requires a target when null.

5 participants

@ccurrens @ealsur @khdang @FabianMeiswinkel @leminh98