dotnet / roslyn

The Roslyn .NET compiler provides C# and Visual Basic languages with rich code analysis APIs.
https://docs.microsoft.com/dotnet/csharp/roslyn-sdk/
MIT License
19.02k stars 4.03k forks source link

CallerArgumentExpression does not capture null-forgiving operator #71390

Open lassevk opened 10 months ago

lassevk commented 10 months ago

Description

When passing an expression as an argument for a parameter, where the method also has a parameter with an [CallerArgumentExpression(...)] attribute referencing said parameter, the null-forgiving operator does not seem to be captured in all cases.

Reproduction Steps

Create a new .NET 8 console application with the following code, and then run it:

using System.Runtime.CompilerServices;

Test(null);
Test(null!);

string s = "Test";
Test(s != null! && s.Length > 0);

void Test(object value, [CallerArgumentExpression(nameof(value))] string? expression = null)
{
    Console.WriteLine(expression);
}

Expected behavior

I expected the following output:

null
null!
s != null! && s.Length > 0

Actual behavior

Instead I get this output:

null
null
s != null! && s.Length > 0

Notice the missing exclamation mark on the second line of the output there, from this statement:

Test(null!);

Also note that the operator is captured inside the bigger expression there (third line of output), so it's not like it is not captured at all.

Regression?

I tested it in .NET 7 and 8 SDKs, with C# 10, 11, and 12, all produces the same output.

Known Workarounds

No response

Configuration

.NET 8, C# 12 (also tested in C# 10 and 11, and .NET 8 and 7 SDKs).

Other information

To be clear, I am not 100% certain this is a bug, it's more of a question really, is this expected behavior, or is there a bug lurking here?

ghost commented 10 months ago

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

Issue Details
### Description When passing an expression as an argument for a parameter, where the method also has a parameter with an `[CallerArgumentExpression(...)]` attribute referencing said parameter, the null-forgiving operator does not seem to be captured in all cases. ### Reproduction Steps Create a new .NET 8 console application with the following code, and then run it: using System.Runtime.CompilerServices; Test(null); Test(null!); string s = "Test"; Test(s != null! && s.Length > 0); void Test(object value, [CallerArgumentExpression(nameof(value))] string? expression = null) { Console.WriteLine(expression); } ### Expected behavior I expected the following output: null null! s != null! && s.Length > 0 ### Actual behavior Instead I get this output: null null s != null! && s.Length > 0 Notice the missing exclamation mark on the second line of the output there, from this statement: Test(null!); ### Regression? I tested it in .NET 7 and 8 SDKs, with C# 10, 11, and 12, all produces the same output. ### Known Workarounds _No response_ ### Configuration .NET 8, C# 12 (also tested in C# 10 and 11, and .NET 8 and 7 SDKs). ### Other information To be clear, I am not 100% certain this is a bug, it's more of a question really, is this expected behavior, or is there a bug lurking here?
Author: lassevk
Assignees: -
Labels: `area-System.Linq.Expressions`
Milestone: -
lassevk commented 10 months ago

the labeler added the wrong tag though, this is not related to System.Linq.Expressions, maybe @jaredpar is more appropriate.

cston commented 10 months ago

Similarly, parentheses are not captured in Test((null)) - see sharplab.io.

CyrusNajmabadi commented 10 months ago

That is odd, and not waht i would expect given the language design we did here. It was always a very syntactic feature, so doing any sort of syntactic processing (like walking into parens/suppression) is very unexpected to me.

lassevk commented 10 months ago

That is odd, and not waht i would expect given the language design we did here. It was always a very syntactic feature, so doing any sort of syntactic processing (like walking into parens/suppression) is very unexpected to me.

To be honest, I expected the feature to be like this:

Test(... insert whatever here ...)

Then the expression parameter would contain exactly ... insert whatever here ..., as long as it compiled. No interpretation, no monkey-business, whatever you wrote, emphasis on wrote, not on "interpreted" or "compiled" or "syntax parsed", would be the contents of that string.

Clearly that is not the case, so either some clarification should be done, ... or maybe there are bugs lurking here.

CyrusNajmabadi commented 10 months ago

@lassevk my expectation based on how we discussed/spec'ed is that it's the raw text of the node, not counting leading/trailing trivia (which relate to roslyn concepts on parsing). So you should get the text of calling .ToString() on the node.

CyrusNajmabadi commented 10 months ago

Looks like we do this:

image

Unfortunately, argument is a BoundExpression which already walked inwards for the actual syntax.

@cston This should likely walk upwards to get the actual expr of hte ArgumentSyntax.

Note: i'm not sure what the spec says for things like ref/in/out/named-args. However, given that we say "caller argument expression" and not "caller argument", i think it would be correct for those to not be part of the final string.

lassevk commented 10 months ago

@lassevk my expectation based on how we discussed/spec'ed is that it's the raw text of the node, not counting leading/trailing trivia (which relate to roslyn concepts on parsing). So you should get the text of calling .ToString() on the node.

That sounds reasonable. The way I understand that comment is that these two should produce the same output:

Test(x);
Test(  x  );

Which again, I find reasonable.

Unfortunately, I don't have enough experience with the roslyn internals to know whether the ToString output would include or should exclude that exclamation mark. I will just point back to my original text saying that I don't know if this is as expected, or if there is a bug here. All I am saying is that it caught me by surprise, so maybe if this is behaving like expected, some documentation clarification is in order.