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

User-defined operator checks should ignore differences in tuple member names #30774

Merged
merged 8 commits into from
Oct 30, 2018
Merged

User-defined operator checks should ignore differences in tuple member names #30774

merged 8 commits into from
Oct 30, 2018

Conversation

RikkiGibson
Copy link
Contributor

@RikkiGibson RikkiGibson commented Oct 26, 2018 •
edited
Loading

@RikkiGibson RikkiGibson requested a review from a team as a code owner October 26, 2018 00:38
@jinujoseph jinujoseph added Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee. labels Oct 26, 2018
@333fred 333fred removed the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Oct 26, 2018
@RikkiGibson
Copy link
Contributor Author

Any reason to prefer one or the other of these:

typeSymbol1.TupleUnderlyingTypeOrSelf() == typeSymbol2.TupleUnderlyingTypeOrSelf()
typeSymbol1.Equals(typeSymbol2, TypeCompareKind.IgnoreTupleNames)

@RikkiGibson RikkiGibson requested a review from a team October 26, 2018 21:46
@RikkiGibson RikkiGibson reopened this Oct 29, 2018
@gafter
Copy link
Member

gafter commented Oct 29, 2018

Any reason to prefer one or the other of these:

typeSymbol1.TupleUnderlyingTypeOrSelf() == typeSymbol2.TupleUnderlyingTypeOrSelf()
typeSymbol1.Equals(typeSymbol2, TypeCompareKind.IgnoreTupleNames)

The latter directly expresses the intent of the code and therefore should be preferred.

gafter
gafter approved these changes Oct 29, 2018
View reviewed changes
Copy link
Member

@gafter gafter left a comment

Choose a reason for hiding this comment

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

:shipit:

@RikkiGibson
Copy link
Contributor Author

Tests pass locally but fail in CI:

++>                 Diagnostic(ErrorCode.WRN_SameFullNameThisAggAgg, "ValueTuple<T1, T2>").WithArguments("", "System.ValueTuple<T1, T2>", "netstandard, Version=2.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51", "System.ValueTuple<T1, T2>").WithLocation(6, 51)
-->                 Diagnostic(ErrorCode.WRN_SameFullNameThisAggAgg, "ValueTuple<T1, T2>").WithArguments("", "System.ValueTuple<T1, T2>", "System.ValueTuple, Version=4.0.1.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51", "System.ValueTuple<T1, T2>").WithLocation(6, 51)

Seems like the environment the compiler is run in affects the exact warning message. Should I change the verification here to skip checking the WRN_SameFullNameThisAggAgg diagnostic?

@RikkiGibson
Copy link
Contributor Author

CI failures related to the build agent getting into a bad state (compiler server is persisting and preventing clearing out/cloning sources at the start of build)

jaredpar
jaredpar approved these changes Oct 30, 2018
View reviewed changes
Copy link
Member

@jaredpar jaredpar left a comment

Choose a reason for hiding this comment

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

:shipit:

@jaredpar
Copy link
Member

@RikkiGibson

Seems like the environment the compiler is run in affects the exact warning message.

This can be the case when error messages mention DLL names. Types can be located in differetn assemblies in desktop and CoreClr.

Which test is failing here? None of the new tests seem to have this message and CI is broken so I couldn't see the failed run you were looking at.

@RikkiGibson
Copy link
Contributor Author

It was TestTupleOperatorConvertToBaseType and TestTupleBinaryOperator. I resolved it in 9f808de by just filtering out the warning diagnostics before verifying.

@RikkiGibson RikkiGibson reopened this Oct 30, 2018
@RikkiGibson RikkiGibson merged commit 6ddf37a into dotnet:master Oct 30, 2018
@RikkiGibson RikkiGibson deleted the symbol-compare-ignore-tuple-names branch October 30, 2018 21:06
wachulski added a commit to wachulski/roslyn that referenced this pull request Oct 31, 2018
@wachulski
Merge remote-tracking branch 'origin/master' into fix/27866/else-with…
4b51565 
…out-if-error-message

* origin/master: (1712 commits)
  User-defined operator checks should ignore differences in tuple member names (dotnet#30774)
  Attempted fix for correctly reporting error CS1069 when using implicit namespaces (dotnet#30244)
  Invert the binding order of InitializerExpressions and CreationExpressions (dotnet#30805)
  Use Arcade bootstrapping scripts (dotnet#30498)
  Ensure that the compilers produce double.NaN values in IEEE canonical form. (dotnet#30587)
  Remove properties set in BeforeCommonTargets.targets
  Fix publishing of dependent projects
  Contributing page: reference Unix build instructions
  Delete 0
  Propagate values from AbstractProject to VisualStudioProjectCreationInfo
  Fix publishing nuget info of dev16.0.x-vs-deps branch
  Revert "Add a SetProperty API for CPS to passing msbuild properties"
  Validate generic arguments in `using static` directives (dotnet#30737)
  Correct 15.9 publish data
  Enable test.
  Do not inject attribute types into .Net modules.
  Add a SetProperty API for CPS to passing msbuild properties
  Revert "add beta2 suffix to dev16 branch"
  Fix references
  Remove commit sha from package versions
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gafter gafter gafter approved these changes

@333fred 333fred 333fred approved these changes

@jaredpar jaredpar jaredpar approved these changes

@AlekseyTs AlekseyTs Awaiting requested review from AlekseyTs

@cston cston Awaiting requested review from cston

Assignees
No one assigned
Labels
Area-Compilers
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@RikkiGibson @gafter @jaredpar @333fred @jinujoseph