Skip to content

Check constraints on lifted operator types#57050

Merged
333fred merged 5 commits intodotnet:mainfrom
333fred:lifted-nullable-ref-struct
Nov 15, 2021
Merged

Check constraints on lifted operator types#57050
333fred merged 5 commits intodotnet:mainfrom
333fred:lifted-nullable-ref-struct

Conversation

@333fred
Copy link
Copy Markdown
Member

@333fred 333fred commented Oct 8, 2021

We weren't validating constraints on lifted operators, which allowed us to generate types such as Nullable<int*> in operator binding without reporting any errors. Fixes #56646. The repro test case for that issue also hit an assert in Binder.ValueCheck.cs, where we were not expecting to run into a BoundConditionalReceiver node. I added it to the list of possible error scenario nodes.

We weren't validating constraints on lifted operators, which allowed us to generate types such as `Nullable<int*>` in operator binding without reporting any errors. Fixes dotnet#56646.
@333fred 333fred requested a review from a team as a code owner October 8, 2021 23:31
@ghost ghost added the Area-Compilers label Oct 8, 2021
@333fred
Copy link
Copy Markdown
Member Author

333fred commented Oct 8, 2021

@dotnet/roslyn-compiler for review.

1 similar comment
@333fred
Copy link
Copy Markdown
Member Author

333fred commented Oct 11, 2021

@dotnet/roslyn-compiler for review.

@AlekseyTs
Copy link
Copy Markdown
Contributor

AlekseyTs commented Oct 11, 2021

I think that under umbrella of this fix we should audit all the places where compiler constructs Nullable types. For example, stand alone conditional access comes to mind, etc. It also feels that, long term, it would be better to keep validity checks close to the code that constructs the types.


In reply to: 940228217

@AlekseyTs
Copy link
Copy Markdown
Contributor

Done with review pass (commit 1)

…operators when lifting would cause invalid type arguments to be supplied to Nullable<T>, rather than erroring after creation. In addition, we now do this for lifted conversions as well, As a part of this, I found and corrected an implementation bug that caused code successfully emitted by the native compiler to emit bad code in Roslyn: we were treating pointer types like value types for the purposes of creating nullable value types, rather than treating them like reference types.
@AlekseyTs
Copy link
Copy Markdown
Contributor

Done with review pass (commit 2). Tests in UserDefinedConversionTests are not looked at.

…-struct

* upstream/main: (829 commits)
  Re-enable BuildWithCommandLine test (dotnet#57584)
  Fix expected formatting in tests
  Simplify
  PR feedback
  Don't crash Use ExpressionBody on local functions in top level statements (dotnet#57571)
  Less linq
  Revert
  Update src/Features/VisualBasic/Portable/NavigationBar/VisualBasicNavigationBarItemService.vb
  Add test
  Simplify
  WIP
  Install .NET SDK in PR Validation pipeline
  Revert to VS2019 for PR Validation
  Update dependencies from https://github.com/dotnet/roslyn build 20211103.8
  Validate the checksum of source on disk (dotnet#57541)
  Clean up VsCodeWindowManager (dotnet#57582)
  Update VS editor package version
  Revert to VS2019 for official build
  Revert to VS2019 for official build
  StackFrame Parser/Lexer as EmbeddedLanguage (dotnet#56957)
  ...
@333fred
Copy link
Copy Markdown
Member Author

333fred commented Nov 8, 2021

@AlekseyTs addressed feedback.


In reply to: 946068806

@AlekseyTs
Copy link
Copy Markdown
Contributor

Done with review pass (commit 4)

Copy link
Copy Markdown
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 5)

@333fred
Copy link
Copy Markdown
Member Author

333fred commented Nov 9, 2021

@dotnet/roslyn-compiler for a second review.

1 similar comment
@333fred
Copy link
Copy Markdown
Member Author

333fred commented Nov 10, 2021

@dotnet/roslyn-compiler for a second review.

@RikkiGibson RikkiGibson self-assigned this Nov 11, 2021
@333fred
Copy link
Copy Markdown
Member Author

333fred commented Nov 15, 2021

/azp run

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 4 pipeline(s).

@333fred 333fred enabled auto-merge (squash) November 15, 2021 19:14
@333fred 333fred merged commit 2da789a into dotnet:main Nov 15, 2021
@ghost ghost added this to the Next milestone Nov 15, 2021
@333fred 333fred deleted the lifted-nullable-ref-struct branch November 15, 2021 23:13
333fred added a commit to 333fred/roslyn that referenced this pull request Nov 17, 2021
…rations

* upstream/main: (3387 commits)
  Fix ValueTracking for index parameters (dotnet#57727)
  Avoid accessing current assembly identity while reporting an accessibility diagnostics for an inaccessible internal symbol. (dotnet#57783)
  Include a type for NoneOperations in VB, print the type in tests (dotnet#57664)
  Don't throw exceptions for file changes after a project is unloaded
  Check up front for being called to remove more than once
  Fix C# language name in spec (dotnet#57427)
  Add test
  Fix null ref in navbars
  Ensure that getting the checksum for a project cone is resilient to its project references being missing
  Check constraints on lifted operator types (dotnet#57050)
  Adjust tests for Windows 11 changes (dotnet#57678)
  Add comment
  Load SVsShellDebugger before calling IVsSolution.CreateSolution
  Remove extra EnsureEditableDocuments  calls (dotnet#57725)
  Don't show nullable annotation in completion items of static field/property
  Don't analyze local function bodies as though they are top level code (dotnet#57623)
  update error code to fix main break (dotnet#57739)
  Error when ref is used on a parameter or return type of an UnmanagedCallersOnly method (dotnet#57043)
  Simplify code from review
  Fix featureflag name for .net 6 host in UI
  ...
@allisonchou allisonchou modified the milestones: Next, 17.1.P2 Nov 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

While applying lifting for operators involving ref structs, compiler can silently construct invalid Nullable types.

4 participants