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

Fixes #22381 - Use in for parameters and arguments #22424

Merged
merged 11 commits into from
Sep 29, 2017
Merged

Fixes #22381 - Use in for parameters and arguments #22424

merged 11 commits into from
Sep 29, 2017

Conversation

OmarTawfik
Copy link
Contributor

@OmarTawfik OmarTawfik commented Sep 29, 2017 •
edited
Loading

Fixes #22381 - Use in for parameters and arguments

@OmarTawfik OmarTawfik added this to the 15.5 milestone Sep 29, 2017
@OmarTawfik OmarTawfik added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Sep 29, 2017
@OmarTawfik OmarTawfik removed the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Sep 29, 2017
@OmarTawfik OmarTawfik changed the title Implement latest LDM design decisions Fixes #22381 - Use in for parameters and arguments Sep 29, 2017
@OmarTawfik OmarTawfik requested review from VSadov, jcouv and a team September 29, 2017 12:06
@OmarTawfik
Copy link
Contributor Author

cc @VSadov @jcouv @dotnet/roslyn-compiler

VSadov
VSadov reviewed Sep 29, 2017
View reviewed changes
src/Compilers/CSharp/Portable/Symbols/Source/SourceOrdinaryMethodSymbol.cs
@@ -226,7 +226,7 @@ private void MethodChecks(MethodDeclarationSyntax syntax, Binder withTypeParamsB
{
diagnostics.Add(ErrorCode.ERR_RefExtensionMustBeValueTypeOrConstrainedToOne, location, Name);
}
else if (parameter0RefKind == RefKind.RefReadOnly && parameter0Type.TypeKind != TypeKind.Struct)
else if (parameter0RefKind == RefKind.In && parameter0Type.TypeKind != TypeKind.Struct)
{
diagnostics.Add(ErrorCode.ERR_RefReadOnlyExtensionMustBeValueType, location, Name);
Copy link
Member

@VSadov VSadov Sep 29, 2017 •
edited by OmarTawfik
Loading

Choose a reason for hiding this comment

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

Should this be ERR_InExtensionMethod. . . ? #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. I'll rename it in a later PR.

In reply to: 141921251 [](ancestors = 141921251)

VSadov
VSadov approved these changes Sep 29, 2017
View reviewed changes
Copy link
Member

@VSadov VSadov left a comment

Choose a reason for hiding this comment

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

LGTM

jcouv
jcouv reviewed Sep 29, 2017
View reviewed changes
src/Compilers/Core/Portable/PublicAPI.Unshipped.txt
@@ -131,6 +131,7 @@ Microsoft.CodeAnalysis.OperationKind.VariableDeclaration = 1030 -> Microsoft.Cod
Microsoft.CodeAnalysis.OperationKind.VariableDeclarationStatement = 3 -> Microsoft.CodeAnalysis.OperationKind
Microsoft.CodeAnalysis.OperationKind.YieldBreakStatement = 12 -> Microsoft.CodeAnalysis.OperationKind
Microsoft.CodeAnalysis.OperationKind.YieldReturnStatement = 16 -> Microsoft.CodeAnalysis.OperationKind
Microsoft.CodeAnalysis.RefKind.In = 3 -> Microsoft.CodeAnalysis.RefKind
Microsoft.CodeAnalysis.RefKind.RefReadOnly = 3 -> Microsoft.CodeAnalysis.RefKind
Copy link
Member

@jcouv jcouv Sep 29, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Should RefKind.RefReadonly be removed? Or is that covered by Vlad's PR? #Closed

Copy link
Member

Choose a reason for hiding this comment

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

Nevermind, we need it for returns...

In reply to: 141945392 [](ancestors = 141945392)

@dpoeschl
Copy link
Contributor

Tagging @dotnet/roslyn-ide as an FYI.

dpoeschl
dpoeschl approved these changes Sep 29, 2017
View reviewed changes
Copy link
Contributor

@dpoeschl dpoeschl left a comment

Choose a reason for hiding this comment

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

The IDE parts LGTM.

jcouv
jcouv approved these changes Sep 29, 2017
View reviewed changes
Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

LGTM and approved for 15.5
Thanks Omar!

@jcouv
Copy link
Member

jcouv commented Sep 29, 2017

ubuntu_14_debug_prtest failed with error below. (details)

error CS0234: The type or namespace name 'Loader' does not exist in the namespace 'System.Runtime' (are you missing an assembly reference?) [/mnt/j/workspace/dotnet_roslyn/master/ubuntu_14_debug_prtest/src/Scripting/Core/Scripting.csproj]

FYI @dotnet/roslyn-infrastructure I'll re-run

@jcouv
Copy link
Member

jcouv commented Sep 29, 2017

test ubuntu_14_debug_prtest please

@jasonmalinowski
Copy link
Member

@jcouv @brettfo Do we have something tracking that as a flaky failure? "missing type" seems unlikely as a flaky anything.

@VSadov
Copy link
Member

VSadov commented Sep 29, 2017

I will merge this since it is passing all tests and I need to rebase onto this change.

@VSadov VSadov merged commit 1fd9a04 into dotnet:master Sep 29, 2017
@OmarTawfik OmarTawfik deleted the remove-ref-readonly-parameters branch September 29, 2017 20:23
@OmarTawfik OmarTawfik mentioned this pull request Sep 29, 2017
Merged
@khyperia khyperia mentioned this pull request Oct 9, 2017
Closed
@OmarTawfik OmarTawfik mentioned this pull request Nov 7, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dpoeschl dpoeschl dpoeschl approved these changes

@VSadov VSadov VSadov approved these changes

@jcouv jcouv jcouv approved these changes

Assignees
No one assigned
Labels
Approved to merge Area-Compilers Area-Language Design cla-already-signed New Language Feature - Readonly References Readonly References
Projects
None yet
Milestone
15.5
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@OmarTawfik @dpoeschl @jcouv @jasonmalinowski @VSadov @dnfclas