Fix resolving types with primary constructors using in parameters

[pseudonym117]

Fixes #105924

in primary constructor parameters are treated as reference types when inspected. Fix here is simply to use the referenced type instead when resolving parameters.

[pseudonym117]

@dotnet-policy-service agree

[hez2010]

Can you elaborate on why you want to inject a service with in? I didn't see any benefit of using in for service injection.
Also, this doesn't seem to be a correct fix as it ends up passing an r-value to a ref parameter.

[pseudonym117]

I would ask the inverse of that - is there any reason why in parameters should not be supported by injection?

Actual use-case here was that I wanted to both use a primary constructor for simplicity, and wanted injected services to be readonly, as mutability of these services could only cause errors.

It is possible that this is not the most correct fix - anecdotally it does resolve the issue for my use-case though. Is there a more correct way to detect in parameters vs ref parameters via reflection? I could not find a better option, but it is very possible that I simply overlooked this. I believe at least for the in use-case, the passing of an r-value should not actually affect usage. However, I can see how this could negatively affect real ref parameters.

[tannergooding]

and wanted injected services to be readonly, as mutability of these services could only cause errors.

in doesn't give you this.

in should be thought of as ref readonly, the only difference is that it allows (does not warn) for rvalues; meaning M(5) is allowed for in but warns for ref readonly and errors for ref/out.

---

A value type T is just T.

A reference type T is a reference (indirection) to the T. In plainer terms, its most like T* in C/C++

A ref T adds one level of indirection. So for a value type T it works like T*; while for a reference type you basically get T**

An out T works the same as a ref T, but requires the value be initialized before first access.

ref readonly T also works like ref T, but doesn't allow the direct contents of T to be mutated.

in T works like ref readonly T, but allows "rvalues", meaning values without a location to be passed. A temporary storage slot is made for them to achieve this.

---

So consider the following

using System;
using System.Diagnostics;

MyClass c = new MyClass();
c.X = 0;
M1(c);
Console.WriteLine(c.X);   // prints 1

M2(c);
Console.WriteLine(c.X);   // prints 2

MyStruct s = new MyStruct();
s.X = 0;
s.C = new MyClass();
s.C.X = 0;
M3(s);
Console.WriteLine(s.X);   // prints 0
Console.WriteLine(s.C.X); // prints 1

M4(s);
Console.WriteLine(s.X);   // prints 0
Console.WriteLine(s.C.X); // prints 2

void M1(in MyClass c)
{    
    // This is disallowed, CS8331
    // c = new MyClass();

    // This is allowed, because we're not mutating 'c', we're mutating the data 'c' refers to
    c.X = 1;
}

void M2(MyClass c)
{    
    // This is allowed, because we're not mutating 'c', we're mutating the data 'c' refers to
    c.X = 2;
    
    // This is allowed
    c = new MyClass();
    
    // This is allowed, but won't be visible externally because we're no longer referring to the same data as the caller
    c.X = 3;
}

void M3(in MyStruct s)
{
    // This is disallowed, CS8331
    // s = new MyStruct();

    // This is disallowed, CS8332
    // s.X = 1;
    
    // This is disallowed, CS8332
    // s.C = new MyClass();

    // This is allowed, because we're not mutating 's', we're mutating the data 's.C' refers to
    s.C.X = 1;
}

void M4(MyStruct s)
{
    // This is allowed, but won't be visible because s is a copy of the data the caller has
    s.X = 1;
    
    // This is allowed, and will be visible because even though its a copy, the reference still points to the same data as the caller
    s.C.X = 2;
    
    // This is allowed
    s = new MyStruct();

    // s was overwritten with a new instance, so s.C is null now
    Debug.Assert(s.C == null);
    s.C = new MyClass();
    
    // This is allowed and won't be visible, because it refers to a new location
    s.C.X = 3;
}

public class MyClass
{
    public int X { get; set; }
}

public struct MyStruct
{
    public int X { get; set; }
    public MyClass C { get; set; }
}

So in is just adding needless cost and not giving you any protection for reference types. It also adds no protection for value types, it is merely a low level perf optimization to avoid the copy overhead of the value type while also preserving the general semantics that mutations of the passed data are not visible to the caller.

[pseudonym117]

Thanks for the detailed explanation - I definitely learned a bit here.

However, I believe you may be overlooking the behavior of in when used in a primary constructor. From what I understand, usage of in (as well as ref and out) in a primary constructor will not auto-generate a field for the parameter, and will only allow usage during class initialization. This means that this code does not compile:

class Dep() { }
class TestClass(in Dep a)
{
  void SomeMethod()
  {
    // CS9109 and CS8331 both prevent this from compiling
    a = new Dep();
  }
}

Without in, this code is completely valid, but error-prone, as injected members should typically be readonly. By adding in, this type of error cannot occur.

this pattern does require adding private readonly Dep a = a; to the class, as referencing an in/out/ref parameter from a primary constructor is not allowed in instance methods

[tannergooding]

However, I believe you may be overlooking the behavior of in when used in a primary constructor

The behavior doesn't change, it is still taking the value byref and still blocking the same scenarios. Not using in also remains consistent with regards to regular parameters.

in T is not the same as readonly T and there is no feature for readonly T in method parameters or primary constrctors today

[pseudonym117]

Yes, but injecting and assigning as readonly can be implemented trivially as:

class Dep() { }
class TestClass(in Dep a)
{
  private readonly Dep _a = a;
  void SomeMethod()
  {
    // will be readonly
    _a.Equals(_a);

    // will not compile as it is not in scope
    a.Equals(a);
  }
}

Implementing readonly function parameters does seem like a good long-term decision in the C# language, but that seems like a far larger scope than is necessary to support this use-case.

In either case, I think that my question from above still stands:

is there any reason why in parameters should not be supported by injection?

[steveharter]

We should not be checking into main for V9. See #105924 (comment).

[tannergooding]

Yes, but injecting and assigning as readonly can be implemented trivially as:

As indicated above, this doesn't do what you think it does and doesn't add any actual protection, its just adding unnecessary cost to the callsite. The right thing to do is simply:

class Dep() { }
class TestClass(Dep a)
{
  private readonly Dep _a = a;
}

In either case, I think that my question from above still stands:

For proper usages of in, probably not. But that entails such scenarios being reasoned about. That is, it shouldn't be driven based on cases where developers misunderstand what in means and the protection it does or does not provide.

[pseudonym117]

We should not be checking into main for V9. See #105924 (comment).

Is there a different branch this can target to be included in V10?

As indicated above, this doesn't do what you think it does and doesn't add any actual protection, its just adding unnecessary cost to the callsite. The right thing to do is simply:

The example you provided does not solve my concern, and in fact makes user-errors more likely. While this does create a readonly property, without the in modifier, the following code will compile, as both a and _a are available in all function scopes:

class Dependency { };
class Dependent(Dependency a)
{ 
  private readonly Dependency _a = a;

  public void UseWrongInstance()
  {
    a = new Dependency();
  }
};

I would argue that this pattern actually makes human-errors more likely, as there will be 2 variables, typically with near-identical names, in scope for IntelliSense to recommend.

For proper usages of in, probably not. But that entails such scenarios being reasoned about. That is, it shouldn't be driven based on cases where developers misunderstand what in means and the protection it does or does not provide.

So from what I understand, you agree that the DependencyInjection library should support injection of in parameters? Assuming that I understood correctly, then further discussion of my specific use-case seems irrelevant to this PR; even if I am completely incorrect in my understanding and usage of in, it should be supported by the library because other valid use-cases exist.

[tannergooding]

So from what I understand, you agree that the DependencyInjection library should support injection of in parameters?

Not quite. The sentiment is that some valid scenario may exist, in which case it may be valid to add that support. But that support should be considered in the context of a valid and real world usage scenario. Such a scenario needs to be explicitly detailed with the relevant rational, tests, etc.

Adding features/support should not be considered or driven with regards to non-typical usage scenarios; such as using in as means to force the primary constructor parameter to not be in scope in other contexts. -- Noting as well that the way you're trying to use it here may also not always do what you want. ref structs can capture ref fields for example and the compiler could theoretically allow capturing of them for classes or structs in the future.

I would argue that this pattern actually makes human-errors more likely, as there will be 2 variables, typically with near-identical names, in scope for IntelliSense to recommend.

This is no different than existing constructor parameters or private fields/public properties which have existed for the 20+ year history of C#/.NET. Auto properties were not always a thing, even with auto properties you often need logic on top and so it is extremely typical to have one, both, or all of a, _a, A, etc.

in is not designed for the scenario you're trying to use it for, and has concrete negative side effects that have been enumerated above. It also goes against the general "Framework Design Guidelines". While these are guidelines and do not have to be followed, they do have very good rationale for why each guideline exists ranging from perf, to the ability to version the API, to meeting user expectations around the API, what they do, how they behave, etc.

The "correct" solution here would be to write an analyzer that flags the usage you don't want to have happen in your codebase. The next best thing would be to log a proposal for a new feature on https://github.com/dotnet/csharplang detailing that you'd like a way to specify that primary constructor variables are only available for field/property assignment and are not available in derived contexts, as you'd like to enforce that users go through the field/property instead.

[steveharter]

Closing; see #105924 (comment)