Skip to content

Add a test for Expression.Property and its handling of property accessors#54279

Merged
eerhardt merged 3 commits intodotnet:mainfrom
vitek-karas:ExpressionPropertyTrimmerTest
Jun 17, 2021
Merged

Add a test for Expression.Property and its handling of property accessors#54279
eerhardt merged 3 commits intodotnet:mainfrom
vitek-karas:ExpressionPropertyTrimmerTest

Conversation

@vitek-karas
Copy link
Member

This mostly a linker test as this effectively validates linker feature from dotnet/linker#1880.

But having a clean trimming tests here is better validation for this specific case.

…sors

This mostly a linker test as this effectively validates linker feature from dotnet/linker#1880.

But having a clean trimming tests here is better validation for this specific case.
@vitek-karas vitek-karas added area-System.Linq.Expressions linkable-framework Issues associated with delivering a linker friendly framework labels Jun 16, 2021
@vitek-karas vitek-karas requested a review from eerhardt June 16, 2021 15:20
@vitek-karas vitek-karas self-assigned this Jun 16, 2021
@ghost
Copy link

ghost commented Jun 16, 2021

Tagging subscribers to this area: @cston
See info in area-owners.md if you want to be subscribed.

Issue Details

This mostly a linker test as this effectively validates linker feature from dotnet/linker#1880.

But having a clean trimming tests here is better validation for this specific case.

Author: vitek-karas
Assignees: vitek-karas
Labels:

area-System.Linq.Expressions, linkable-framework

Milestone: -

{
var obj = new TestType();
var param = Expression.Parameter(typeof(TestType));
var prop = Expression.Property(param, typeof(TestType).GetMethod("get_TestProperty"));
Copy link
Member

Choose a reason for hiding this comment

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

Maybe it is overkill, but should we also have a test that has Roslyn creating the Expression? ex.

        var obj = new TestType();
        
        Expression<Func<TestType, bool>> expression = t => t.TestProperty;
        var prop = ((MemberExpression)expression.Body);
        ((PropertyInfo)prop.Member).SetValue(obj, true);

        if (obj._testPropertyValue != true)
            return -1;

        return 100;

Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

Thanks for following up on this.

@eerhardt
Copy link
Member

eerhardt commented Jun 16, 2021

But having a clean trimming tests here

Note: we don't verify our trimming tests don't have ILLink warnings.

<SuppressTrimAnalysisWarnings>true</SuppressTrimAnalysisWarnings>

But maybe we should have some subset of tests that verify no warnings?

@vitek-karas
Copy link
Member Author

It would be nice to be able to validate that there were no warnings. I looking at that a little bit, but it's rather complicated... not a simple fix anywhere really.


result = LambdaCreation.Test();

Type.GetType("foo");
Copy link
Member

Choose a reason for hiding this comment

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

??

Copy link
Member Author

Choose a reason for hiding this comment

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

I was trying to turn on the warnings - and this is an easy way to get a warning... but I left it in.

@eerhardt
Copy link
Member

Linker tests passed. Runtime libraries test failures are unrelated.

@eerhardt eerhardt merged commit 1306b03 into dotnet:main Jun 17, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jul 17, 2021
@vitek-karas vitek-karas deleted the ExpressionPropertyTrimmerTest branch September 26, 2023 09:22
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.Linq.Expressions linkable-framework Issues associated with delivering a linker friendly framework

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants