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

Missing test coverage for CustomConstantAttribute #49748

Closed
MichalStrehovsky opened this issue Mar 17, 2021 · 6 comments
Closed

Missing test coverage for CustomConstantAttribute #49748

MichalStrehovsky opened this issue Mar 17, 2021 · 6 comments
Labels
area-System.Reflection good first issue Issue should be easy to implement, good for first-time contributors help wanted [up-for-grabs] Good issue for external contributors
Milestone

Comments

@MichalStrehovsky
Copy link
Member

This is used in e.g. reflection invoke and Delegate.DynamicInvoke. The below program will print "Hello". We should ideally turn this into a test and add it somewhere around

public static void DynamicInvoke_MissingTypeForDefaultParameter_Succeeds()
.

using System;
using System.Runtime.CompilerServices;


Mine m = MyAttribute.Do;
m.DynamicInvoke(Type.Missing);

delegate void Mine([My] object o);

class MyAttribute : CustomConstantAttribute
{
    public static void Do(object o)
    {
        Console.WriteLine(o);
    }

    public override object Value => "Hello";
}
@MichalStrehovsky MichalStrehovsky added area-System.Reflection good first issue Issue should be easy to implement, good for first-time contributors help wanted [up-for-grabs] Good issue for external contributors labels Mar 17, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Mar 17, 2021
@SkiFoD
Copy link
Contributor

SkiFoD commented Mar 17, 2021

Hey, I'd love to work with the issue. If you don't mind I will take it.

@MichalStrehovsky
Copy link
Member Author

It's yours! Thank you!

SkiFoD added a commit to SkiFoD/runtime that referenced this issue Mar 17, 2021
There is a test the custom constant attribute assigns the default value to Type.Missing parameter.
SkiFoD added a commit to SkiFoD/runtime that referenced this issue Mar 18, 2021
MichalStrehovsky pushed a commit that referenced this issue Mar 21, 2021
There is a test the custom constant attribute assigns the default value to Type.Missing parameter.
@buyaa-n buyaa-n added this to the Future milestone Mar 22, 2021
@buyaa-n buyaa-n removed the untriaged New issue has not been triaged by the area owner label Mar 22, 2021
@buyaa-n
Copy link
Contributor

buyaa-n commented Mar 22, 2021

@MichalStrehovsky @SkiFoD seems it is fixed with #49767? Could we close it?

@ghost ghost added the needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration label Mar 22, 2021
@buyaa-n buyaa-n removed the needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration label Mar 22, 2021
@MichalStrehovsky
Copy link
Member Author

Yup, closing.

@ghost ghost locked as resolved and limited conversation to collaborators Apr 21, 2021
@ivanpovazan
Copy link
Member

Hello @MichalStrehovsky

I know this is an closed issue but is related to what I am trying to fix for Mono via #75612 .
After looking into code in coreCLR, I am trying to figure out what is the defined behaviour in case when there are multipleCustomConstant (or derived) attributes attached to a parameter.

Based on this comment: https://github.com/dotnet/runtime/blob/79ae74f5ca5c8a6fe3a48935e85bd7374959c570/src/coreclr/System.Private.CoreLib/src/System/Reflection/RuntimeParameterInfo.cs#L311-#L317 and code that follows, it seems coreCLR would handle default value resolution differently when there are more than one CustomConstant attribute attached (depending on their type: DateTime vs decimal).

Do you have any insight on what would be the desired behaviour?

PS I am also planning to improve test coverage for such cases.

@MichalStrehovsky
Copy link
Member Author

Do you have any insight on what would be the desired behaviour?

The defined behavior is basically how CoreCLR implemented it because it has been like that for 20 years.

It also looks like there's a difference in behavior for DefaultValue and RawDefaultValue (one seems to pick the first CustomConstant, the other will pick the last one). Fun. Thanks for looking into it!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Reflection good first issue Issue should be easy to implement, good for first-time contributors help wanted [up-for-grabs] Good issue for external contributors
Projects
No open projects
Development

No branches or pull requests

4 participants