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

Accept PE Operator Symbols having in parameters #23508

Merged
merged 2 commits into from
Dec 10, 2017
Merged

Accept PE Operator Symbols having in parameters #23508

merged 2 commits into from
Dec 10, 2017

Conversation

OmarTawfik
Copy link
Contributor

@OmarTawfik OmarTawfik commented Dec 1, 2017 •
edited
Loading

Customer scenario

// ref.dll
public class Test
 {
     public int Value { get; set; }
 
     public static int operator +(in Test a, in Test b)
     {
         return a.Value + b.Value;
     }
 }
// main.exe
 class Program
 {
     static void Main(string[] args)
     {
         var a = new Test { Value = 3 };
         var b = new Test { Value = 6 };
 
         System.Console.WriteLine(a + b);
     }
 }

Expected: prints 9
Actual: CS0019: Operator '+' cannot be applied to operands of type 'Test' and 'Test'

Bugs this fixes

Fixes devdiv bug 530136 (feedback link)

Workarounds, if any

None needed

Risk

Low.

Performance impact

None.

Is this a regression from a previous update?

No.

Root cause analysis

It was an implementation detail in how C# compiler imports symbols from other binaries. Now fixed.
The compiler would ignore operators that have parameters with any ref kind.

How was the bug found?

Customer report.

Test documentation updated?

No.

@OmarTawfik OmarTawfik added Area-Compilers Bug PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. labels Dec 1, 2017
@OmarTawfik OmarTawfik added this to the 15.6 milestone Dec 1, 2017
@OmarTawfik
Copy link
Contributor Author

OmarTawfik commented Dec 1, 2017 •
edited
Loading

@dotnet/roslyn-infrastructure flaky test: https://ci.dot.net/job/dotnet_roslyn/job/master/job/windows_release_unit64_prtest/10933/testReport/junit/Microsoft.CodeAnalysis.Editor.UnitTests.Workspaces/ProjectCacheHostServiceFactoryTests/TestImplicitCacheMonitoring/ #Closed

@OmarTawfik OmarTawfik removed the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Dec 1, 2017
VSadov
VSadov approved these changes Dec 2, 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
Thanks!!

VSadov
VSadov reviewed Dec 2, 2017
View reviewed changes
src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEMethodSymbol.cs
!this.IsParams();
private bool IsValidUserDefinedOperatorSignature(int parameterCount)
{
if (this.ReturnsVoid || this.IsGenericMethod || this.IsVararg || this.ParameterCount != parameterCount || this.IsParams())
Copy link
Member

@VSadov VSadov Dec 2, 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.

Please enter a bug on rejecting ref-returning operators (defined in IL). It seems we have always allowed them, so there could be compat concerns.
Also it is not clear whether they "just worked", or were broken anyways. #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.

Filed #23606 for that.

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

@OmarTawfik
Copy link
Contributor Author

OmarTawfik commented Dec 4, 2017 •
edited
Loading

@dotnet/roslyn-compiler can I get another view? #Closed

jaredpar
jaredpar reviewed Dec 5, 2017
View reviewed changes
src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenInParametersTests.cs
}";

CompileAndVerify(code, additionalRefs: new[] { reference.ToMetadataReference() }, expectedOutput: "3");
CompileAndVerify(code, additionalRefs: new[] { reference.EmitToImageReference() }, expectedOutput: "3");
Copy link
Member

@jaredpar jaredpar Dec 5, 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 we have tests to ensure that we import both the in and normal operators? That should result in an ambiguity error (as discussed in LDM) but we should have a test for it. #Pending

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With the merge of #23122 it will no longer be an ambiguity, and we will choose the by-val one. Did we decide otherwise? why would overload resolution over PE symbols differ than source symbols?

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jaredpar can you please comment on the LDM decision?

Copy link
Member

Choose a reason for hiding this comment

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

You are correct: we prefer the val version here. I had the LDM decision backward when I made my comment.

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Dec 5, 2017 •
edited
Loading

Should any changes be made for VB? #Closed

AlekseyTs
AlekseyTs reviewed Dec 5, 2017
View reviewed changes
src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenInParametersTests.cs
{
public int Value { get; set; }

public static int operator +(in Test a, in Test b)
Copy link
Contributor

@AlekseyTs AlekseyTs Dec 5, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

public static int operator +(in Test a, in Test b) [](start = 4, length = 50)

Please add a test for scenarios when in is only on the left and in is only on the right. #Closed

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Dec 5, 2017 •
edited by OmarTawfik
Loading

Done with review pass (iteration 1) #Closed

@OmarTawfik
Copy link
Contributor Author

I don't think there are any plans to support them for now. I've added tests nonetheless.

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

AlekseyTs
AlekseyTs approved these changes Dec 7, 2017
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 (iteration 2).

@OmarTawfik OmarTawfik requested a review from a team as a code owner December 7, 2017 20:20
@OmarTawfik
Copy link
Contributor Author

@MeiChin-Tsai for ask mode approval.

@MeiChin-Tsai
Copy link

Approved. Please make sure all checks are green.

@jcouv
Copy link
Member

jcouv commented Dec 8, 2017

test ubuntu_14_debug_prtest please

@jcouv
Copy link
Member

jcouv commented Dec 8, 2017

ubuntu_16_debug_prtest failed with error below.

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

```/tmp/jenkins4339886900905968752.sh: 2: /tmp/jenkins4339886900905968752.sh: ./build/scripts/cibuild.sh: Permission denied`

@jcouv
Copy link
Member

jcouv commented Dec 8, 2017

test ubuntu_16_debug_prtest please

@jcouv
Copy link
Member

jcouv commented Dec 8, 2017

Ubuntu issue seems persistent

@OmarTawfik
Copy link
Contributor Author

@dotnet/roslyn-infrastructure is this a known issue?
10:00:09 /tmp/jenkins6587034164630223535.sh: 2: /tmp/jenkins6587034164630223535.sh: ./build/scripts/cibuild.sh: Permission denied
https://ci.dot.net/job/dotnet_roslyn/job/master/job/ubuntu_14_debug_prtest/7963/console

@OmarTawfik
Copy link
Contributor Author

@dotnet-bot test ubuntu_14_debug_prtest
@dotnet-bot test ubuntu_16_debug_prtest

@jaredpar
Copy link
Member

jaredpar commented Dec 8, 2017

@khyperia did you make sure to mark that script as executable?

@OmarTawfik
Copy link
Contributor Author

@jaredpar that seems to be a very odd diff. I'll rebase and make sure it is out of the commit.

@khyperia
Copy link
Contributor

khyperia commented Dec 8, 2017

@khyperia did you make sure to mark that script as executable?

It's definitely executable in latest master, pretty sure it's been that way...

@khyperia
Copy link
Contributor

khyperia commented Dec 8, 2017

Ah, yeah, this PR has this:

image

(I assume that's what @OmarTawfik meant with their latest comment)

@OmarTawfik
Copy link
Contributor Author

Thanks @khyperia . Something wrong went in that merge. Rebased and should be passing now.

@OmarTawfik OmarTawfik merged commit 1688be0 into dotnet:master Dec 10, 2017
@OmarTawfik OmarTawfik deleted the fix-in-operators-from-metadata branch December 10, 2017 08:48
@OmarTawfik OmarTawfik mentioned this pull request Dec 10, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@VSadov VSadov VSadov approved these changes

@jaredpar jaredpar jaredpar left review comments

@AlekseyTs AlekseyTs AlekseyTs approved these changes

Assignees

@OmarTawfik OmarTawfik

Labels
Approved to merge Area-Compilers Bug
Projects
None yet
Milestone
15.6
Development

Successfully merging this pull request may close these issues.
7 participants
@OmarTawfik @AlekseyTs @MeiChin-Tsai @jcouv @jaredpar @khyperia @VSadov