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

Type test for ref struct is incorrect #63085

Closed
iskiselev opened this issue Jul 29, 2022 · 12 comments · Fixed by #63646
Closed

Type test for ref struct is incorrect #63085

iskiselev opened this issue Jul 29, 2022 · 12 comments · Fixed by #63646
Assignees
Labels
4 - In Review A fix for the issue is submitted for review. Area-Compilers Bug
Milestone

Comments

@iskiselev
Copy link

iskiselev commented Jul 29, 2022

If ref struct G<T> defined, this type test inside the struct for any resolved generic returns false (and compiler not even add test in IL).
It also shows warning CS0184 and does not allow to use it in pattern math type test.

Version Used: C# 10

Steps to Reproduce:

Compile and run next code:

Use next code:

using System;

new G<int>().Test();
new G<object>().Test();

ref struct G<T>
{
    public void Test()
    {
        if (this is G<int>)
        {
            Console.WriteLine("int");
        }
        else if (this is G<object>)
        {
            Console.WriteLine("object");
        }
        else
        {
            Console.WriteLine("unknown");
            Console.WriteLine(typeof(T));
        }
    }

    /*public void TestPattern()
    {
        var genericTypePattern = this switch
        {
            G<int> => "int",
            G<object> => "object",
            _ => "unknown"
        };

        Console.WriteLine(genericTypePattern);
    }*/
}

On SharpLib.IO

Expected Behavior:
Program should output:

int
object

No CS0184 warning should not be shown, if uncomment, TestPattern method should compile without errors.

Actual Behavior:
Program outputs:

unknown
System.Int32
unknown
System.Object

CS0184 warning shown, TestPattern method can not be compiled without changes

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Jul 29, 2022
@jcouv jcouv added Bug and removed untriaged Issues and PRs which have not yet been triaged by a lead labels Aug 2, 2022
@jcouv jcouv added this to the 17.4 milestone Aug 2, 2022
@jaredpar jaredpar assigned AlekseyTs and unassigned cston Aug 24, 2022
@AlekseyTs
Copy link
Contributor

In order to perform a runtime type test, a value type must be boxes, but ref-structs cannot be boxed. Perhaps instead of producing a warning for is operator, we should be producing an error rather than assuming that an unsupported type test always fails?

@AlekseyTs
Copy link
Contributor

@jaredpar, @MadsTorgersen What do you think?

@MadsTorgersen
Copy link
Contributor

I think it should be an error. That way we preserve our ability to do it right later without changing semantics.

Also the current result is just plain wrong!

@jaredpar
Copy link
Member

@AaronRobinsonMSFT @davidwrighton

In order to perform a runtime type test, a value type must be boxes, but ref-structs cannot be boxed

This is a good point. Is there a way to safely type test a ref struct when generics are involved like this?

I think it should be an error. That way we preserve our ability to do it right later without changing semantics.

Generally agree with this in the short term.

@AaronRobinsonMSFT

This comment was marked as off-topic.

@AaronRobinsonMSFT
Copy link
Member

Boo. I didn't realize these all compile down to IL. @jaredpar I get your question now.

@AlekseyTs
Copy link
Contributor

@AaronRobinsonMSFT Scenario that you are trying is different and compiler behaves differently. It handles identity case specially, I think.

@iskiselev
Copy link
Author

I'd like to share some context how I come to this issue, if it will help.
I've implemented custom InterpolatedStringHandler for my logger. The logger has interface with separate Trace, Debug, Info, ..., methods. The handler should return false if current log level is bellow level that will be written by logger. As the only different thing is LogLevel, I thought I will be able to move it to generic class function and will have something like:

interface ILogger {
 void Trace([InterpolatedStringHandlerArgument("")]LogInterpolatedHandler<TraceLevel> handler);
 void Debug([InterpolatedStringHandlerArgument("")]LogInterpolatedHandler<DebugLevel> handler);
...
}

I come up with if block typeof(T) == typeof(TraceLevel) and so on (as suggested in previous comment by @davidwrighton), but it was slower, than using just separate class for interpolated handlers. I thought that with this test it may work faster and, probably, JIT will be able to eliminate check and non-used branches, if ...Level classes would be value types.
I believe my original problem would be solved by abstract interface method (with code used in #63617), but it would not work on older runtimes (while interpolated string handler works perfect).

@AaronRobinsonMSFT
Copy link
Member

Seems we have 2 questions here.

  1. Can this be done at present? Yes, but one needs to use typeof(G<T>) == ... instead of is which is a potential boxing IL sequence operation when the "might be" equivalence is present.

  2. Do we have an IL sequence to express this that will be elided at run-time? At present no, but once we fully support ByRefLike types as generic parameters we will have a sequence. See TypeSystem: Support ref structs within generics runtime#65112.

@jkotas
Copy link
Member

jkotas commented Aug 26, 2022

typeof(G<T>) == ... is recognized by the JIT optimizations and elided where possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4 - In Review A fix for the issue is submitted for review. Area-Compilers Bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants