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.

Sign up for GitHub

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

Jump to bottom

Add missing test for CallerArgumentExpression #57805

Merged
merged 4 commits into from
Nov 30, 2021
Merged

Add missing test for CallerArgumentExpression #57805

merged 4 commits into from
Nov 30, 2021

Conversation

333fred
Copy link
Member

@333fred 333fred commented Nov 16, 2021

Closes #52745.

@333fred 333fred requested a review from a team as a code owner November 16, 2021 21:36
@333fred
Copy link
Member Author

333fred commented Nov 16, 2021

This test, combined with dotnet/csharplang#5438 to update the spec with the conversion rules, closes out the test plan for CallerArgumentExpression. @dotnet/roslyn-compiler for a very small test-only change.

@333fred 333fred enabled auto-merge (squash) November 16, 2021 22:04
This was referenced Nov 16, 2021
Closed
Closed
AlekseyTs
AlekseyTs reviewed Nov 17, 2021
View reviewed changes
src/Compilers/CSharp/Test/Emit/Attributes/AttributeTests_CallerInfoAttributes.cs
", targetFramework: TargetFramework.NetCoreApp);

comp.VerifyDiagnostics(
// (5,16): error CS8964: The CallerArgumentExpressionAttribute may only be applied to parameters with default values
Copy link
Contributor

@AlekseyTs AlekseyTs Nov 17, 2021 •
edited by 333fred
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

may only be applied to parameters with default values

It feels like we also need to test scenario that satisfies this requirement, even if that would be an error on its own. #Resolved

AlekseyTs
AlekseyTs reviewed Nov 17, 2021
View reviewed changes
src/Compilers/CSharp/Test/Emit/Attributes/AttributeTests_CallerInfoAttributes.cs Outdated
[InlineData("out")]
[InlineData("ref")]
[InlineData("in")]
public void CallerArgumentExpression_OnRefParameter(string refType)
Copy link
Contributor

@AlekseyTs AlekseyTs Nov 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CallerArgumentExpression_OnRefParameter

I think we need to make sure we are testing the same scenario for VB. #Closed

@AlekseyTs
Copy link
Contributor

Done with review pass (commit 1)

@333fred
Copy link
Member Author

333fred commented Nov 17, 2021

@AlekseyTs addressed feedback.

In reply to: 971899219

AlekseyTs
AlekseyTs reviewed Nov 17, 2021
View reviewed changes
src/Compilers/CSharp/Test/Emit/Attributes/AttributeTests_CallerInfoAttributes.cs Outdated
Diagnostic(ErrorCode.ERR_BadCallerArgumentExpressionParamWithoutDefaultValue, "CallerArgumentExpression").WithLocation(5, 16),
// (5,47): error CS1741: A ref or out parameter cannot have a default value
// void M(int i, [CallerArgumentExpression("i")] out string s = null)
Diagnostic(ErrorCode.ERR_RefOutDefaultValue, "out").WithLocation(5, 47)
Copy link
Contributor

@AlekseyTs AlekseyTs Nov 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"out"

Should this be "ref" for ref? #Closed

AlekseyTs
AlekseyTs reviewed Nov 17, 2021
View reviewed changes
src/Compilers/CSharp/Test/Emit/Attributes/AttributeTests_CallerInfoAttributes.cs
{
Console.WriteLine(s);
}
", targetFramework: TargetFramework.NetCoreApp);
Copy link
Contributor

@AlekseyTs AlekseyTs Nov 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

targetFramework: TargetFramework.NetCoreApp

I guess default target framework would work just fine since the test is for CoreClrOnly. #Closed

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it will still compile against Netstandard2.0 APIs, which doesn't have CallerArgumentExpression.

AlekseyTs
AlekseyTs approved these changes Nov 17, 2021
View reviewed changes
Copy link
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (commit 3), assuming CI is passing

@333fred 333fred enabled auto-merge (squash) November 17, 2021 22:22
@runfoapp runfoapp bot mentioned this pull request Nov 17, 2021
Open
@333fred 333fred merged commit e302aa0 into dotnet:main Nov 30, 2021
@ghost ghost added this to the Next milestone Nov 30, 2021
@333fred 333fred deleted the callerargexpr-test branch November 30, 2021 02:55
@allisonchou allisonchou modified the milestones: Next, 17.1.P2 Nov 30, 2021
333fred added a commit to 333fred/roslyn that referenced this pull request Dec 3, 2021
@333fred
Merge remote-tracking branch 'upstream/main' into interp-nullable-imp…
7663d94 
…rovements

* upstream/main: (310 commits)
  Read SourceLink info and call service to retrieve source from there (dotnet#57978)
  Add new parser/lexer to the StackTraceAnalyzer (dotnet#57598) (dotnet#58050)
  Snap 17.1 P2 (dotnet#58041)
  Make it possible to analyze the dataflow of `ConstructorInitializerSyntax` and `PrimaryConstructorBaseTypeSyntax` (dotnet#57576)
  Shorten paths in VS installation (dotnet#57726)
  Add comments
  Add new parser/lexer to the StackTraceAnalyzer (dotnet#57598)
  Fix await completion for expression body lambda
  Add tests
  Fix comment
  Honor option, and also improve formatting with comment
  Skip TestLargeStringConcatenation (dotnet#58035)
  Log runtime framework of remote host
  Mark EqualityContract property accessor as not auto-implemented (dotnet#57917)
  Fix typo in XML doc for GeneratorExtensions (dotnet#58020)
  Hold Receiver directly in bound node for implicit indexer access (dotnet#58009)
  Pass AnalysisKind instead of int
  Enable nullable reference types for TableDataSource
  Simplify 'interpolation' data, and move to an easier to consume System.Range approach for it (dotnet#57966)
  Add missing test for CallerArgumentExpression (dotnet#57805)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RikkiGibson RikkiGibson RikkiGibson approved these changes

@AlekseyTs AlekseyTs AlekseyTs approved these changes

Assignees
No one assigned
Labels
Area-Compilers
Projects
None yet
Milestone
17.1.P2
Development

Successfully merging this pull request may close these issues.

Test plan for CallerArgumentExpressionAttribute
4 participants
@333fred @AlekseyTs @RikkiGibson @allisonchou