Pass constants to parameters marked as [ConstantExpected]

[terrajobst]

Consider adding an [ConstantExpected] attribute that could be put on parameters to indicate that arguments should be constants rather than variables. See #30740 (comment) for details.

The attribute proposal

namespace System.Runtime.CompilerServices
{
    [AttributeUsage(AttributeTargets.Parameter, AllowMultiple = false, Inherited = false)]
    public sealed class ConstantExpectedAttribute : Attribute
    {
        public object? Min { get; set; }
        public object? Max { get; set; }
    }
}

Usage and Diagnostic examples

public static class C
{
    public static void MyBool1([ConstantExpected] bool b) { }
    public static void MyLong1([ConstantExpected] long b) { }
    public static void MyLong2([ConstantExpected(Min = -5, Max = 10)] long b) { }
    public static void MyFloat1([ConstantExpected] float b) { }
    public static void MyFloat2([ConstantExpected(Min = -5.3f, Max = 10.1f)] float b) { }

    // Might want to warn for the negative values and out of range values
    public static void MyInvalidUshort([ConstantExpected(Min = -5, Max = -1)] ushort b) { }
    public static void MyInvalidRange([ConstantExpected(Min = 5, Max = -5)] int b) { }
    // flag any ref type usage as not applicable
    public static void MyString([ConstantExpected] string b) { }

    // Diagnostics examples
    public static void Test(long b, ushort u)
    {
        // OK
        const long a = 10;
        MyLong1(a);
        MyLong2(a);
        MyLong1(1L);
        MyLong2(2L);
        MyInvalidUshort(1);
        const ushort us = 0;
        MyInvalidUshort(us); // Flag

        MyLong1(b); // Flag
        MyLong2(b); // Flag

                
        MyLong2(20); // Flag, out of range
        MyInvalidUshort(u); // Flag
        MyInvalidUshort(10); // Flag, out of the range
    }
}

Category: Reliability
Severity: Info

[jeffhandley]

Estimates:

• Analyzer: Medium
• Fixer: Not Applicable

[jeffhandley]

This issue should capture:

1. The attribute itself (getting that through the API review)
2. Creation of the analyzer
3. Identification of some instances where the attribute should be applied

The work to apply the attribute (especially through intrinsics) could be a separate issue/PR.

[jeffhandley]

@tannergooding Do you think this is a must-have attribute & analyzer for .NET 5 to round out the intrinsics work we've done, or do you think this could be added in .NET 6?

[tannergooding]

It is not. The hardware intrinsics don't even require constant inputs, they just benefit from them and you will get very poor codegen if you don't.
We shipped .NET Core 3.0 without such an attribute and I'm sure we'll be fine if we do the same for .NET 5.

[jeffhandley]

Thank you, @tannergooding. I'm going to move this to Future so that we can consider it for .NET 6 then.

[Mrnikbobjeff]

How would we catch expressions which are evaluated at compile time? I.e. 2*2*2 would be folded to 8 and I guess emit the same code as if we would manually fold it, but manually incorporating folding rules to detect if the expression may become a constant would be difficult or am I missing something?

[carlossanlop]

@GrabYourPitchforks @tannergooding should we have a separate API proposal for the attribute itself? Or in other words, should we reopen #33814 for that purpose specifically? I don't mind if we can get both the attribute and the analyzer approved in one single issue.

Attribute questions

• The name ConstantExpected makes more sense. Levi proposed Immediate in the other issue.

• Here is an example of an API Levi shared in the other issue, which could benefit from having both the new proposed attribute.

namespace System.Runtime.Intrinsics.X86
{
    public abstract class Sse2 : Sse
    {
        public static Vector128<short> ShiftLeftLogical(Vector128<short> value, [ConstantExpected(/* Min = 0, Max = 15 */)] byte count);
    }
}

A proposal of the attribute could be:

    [AttributeUsage(AttributeTargets.Parameter, AllowMultiple = false, Inherited = false)]
    public sealed class ConstantExpectedAttribute : Attribute
    {​​​​
        public int? Min {​​​​ get; set; }​​​​
        public int? Max {​​​​ get; set; }​​​​
    }​​​​

• @GrabYourPitchforks Do we want the attribute to receive limits as arguments, like you added in your example as a comment (Min/Max)? Is the type only expected to be an integer? Is this even possible if we don't know the specific parameter type? If we have constant number types, we could find the default minimum and maximum values, but for other types like string, which could also be constant, it wouldn't make sense to try to find a min/max.

• In which assembly/namespace would the attribute live?

Analyzer questions

• The analyzer should have an Info severity, since it's allowed to pass a non-constant even if the parameter has the attribute. Agreed?

• Could we make the analyzer mandatory for Intrinsics APIs only? And if we could, would be interested in doing that?

[GrabYourPitchforks]

If we wanted min / max, they could be typed as object?. This would allow byte, sbyte, int, double, etc. parameters. This isn't being checked at runtime - only compile-time - and it's straightforward enough for the compiler to handle attribute properties typed as object. For string literals, min / max would stay null.

[tannergooding]

I think that this issue is fine for tracking the API, and I believe that was @terrajobst intent when he tagged it api-needs-work.

[zvrba]

I'm late to the discussion, but in relation to use with intrinsics, I'd prefer something like void DoSomething(int runtimeValue, const byte imm). Passing in anything than a compile-time constant for imm would generate a compile error. This extends the use of keyword to a currently invalid context, so nothing would get broken.

It could possibly be extended to have a generic constraint where T : const int and subsequently one could write "metacode" that unrolls loops at compile-time like one does with templates in C++. But I guess there are source generators for that now.

[tannergooding]

That would be a breaking change and go against the goals of hardware intrinsics in .NET. It also has several issues, including that you couldn't write code that works with values that become constant after inlining.

[wzchua]

I would like to be assigned this.

Does the work start with getting the attribute merged into runtime repo, then writing the analyzer? Are there other steps for the attribute?

[buyaa-n]

Does the work start with getting the attribute merged into runtime repo, then writing the analyzer?

Yes start with the attribute getting merged to the runtime repo, for the analyzer you can use a mock attribute for testing until you were able to consume it in the analyzers repo

Are there other steps for the attribute?

I would say no, but not sure about what exactly you are asking, here is a sample PR for adding an attribute

[buyaa-n]

Also, there could be a step "applying the attribute" to the APIs in the runtime repo but that is not needed to be covered by this issue

[wzchua]

Do we allow composition of different types? Like what should the analyzer do with something like below:

public static void TestMethod([ConstantExpected(Min=0, Max=5)] uint constant)
{
    TestMethod2(constant);
}
public static void TestMethod2([ConstantExpected(Min=0, Max=10)] int val)
{
}

[wzchua]

Also, does casting influence the analyzer?

TestMethod((uint)100);

[buyaa-n]

Do we allow composition of different types? Like what should the analyzer do with something like below:

Not sure I follow the question, though I believe the analyzer should warn on TestMethod2(constant); invocation because it is not passing a constant

public static void TestMethod([ConstantExpected(Min=0, Max=5)] uint constant)
{
    TestMethod2(constant); // should warn here as constant expected for 'val' parameter
}
public static void TestMethod2([ConstantExpected(Min=0, Max=10)] int val)
{
}

Also, does casting influence the analyzer?

TestMethod((uint)100);

Not sure what issue you are having here, but TestMethod((uint)100); and TestMethod(100); in your example should behave the same

[wzchua]

It seems we want to be able to compose the ConstantExpectedAttribute using another ConstantExpectedAttribute. I'm clarifying what should be done if the types don't match.

I guess I'm asking does casting make it not a constant?
It might be more relevant for the composition case.
I guess it is not an issue for the literal scenario.

[buyaa-n]

Good question, tagging @tannergooding @GrabYourPitchforks @bartonjs for answer

[wzchua]

Given,

using System;
using System.Diagnostics.CodeAnalysis;
#nullable enable

public class Test
{
    public interface ITest<T>
    {
        T Add(T operand1, [ConstantExpected] T operand2);
    }
    public abstract class AbstractTest<T>
    {
        public abstract T Add2(T operand1, [ConstantExpected] T operand2);
    }
    public class GenericInt : AbstractTest<int>, ITest<int>
    {
        public int Add(int operand1, int operand2)
        {
            return 0;
        }
        public override int Add2(int operand1, int operand2)
        {
            return 0;
        }
    }
}

I assume the analyzer should warn for not having [ConstantExpected] in the more concrete type parameter?

[tannergooding]

I assume the analyzer should warn for not having [ConstantExpected] in the more concrete type parameter?

I'd expect so, yes. There is some "wonkiness" here though, in that [ConstantExpected] is "impossible" to fulfill for non-primitive types. So if T were a Guid, for example, we'd need to decide what happens.

[wzchua]

Ok, i have written the tests for many of the cases.
For cases where the attribute is applied to T, setting Min Max is an error, and warning at callsite only appears if concrete type for T can be constant

[deeprobin]

dotnet/roslyn-analyzers#5766 (comment)
@bartonjs Do we need a VB analyzer?

/cc @wzchua

[bartonjs]

@deeprobin The general stance is "most analyzers should Just Work for both C# and VB, so you should add VB tests and enjoy that it Just Worked... but if your analyzer requires special VB code it's probably not worth it"