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

Factor nullability logic for placeholders #58036

Merged
merged 5 commits into from
Dec 4, 2021
Merged

Factor nullability logic for placeholders #58036

merged 5 commits into from
Dec 4, 2021

Conversation

jcouv
Copy link
Member

@jcouv jcouv commented Nov 30, 2021 •
edited
Loading

Introduce NullableWalker.AddPlaceholderReplacement and RemovePlaceholderReplacement

@jcouv jcouv self-assigned this Nov 30, 2021
@jcouv jcouv force-pushed the nullable-placeholders branch 4 times, most recently from f841fb3 to 2b93066 Compare November 30, 2021 20:52
@jcouv jcouv force-pushed the nullable-placeholders branch from 2b93066 to c4596a2 Compare November 30, 2021 20:52
@jcouv jcouv marked this pull request as ready for review November 30, 2021 23:04
@jcouv jcouv requested a review from a team as a code owner November 30, 2021 23:04
AlekseyTs
AlekseyTs reviewed Dec 1, 2021
View reviewed changes
src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs Outdated
@@ -4177,13 +4218,22 @@ private void LearnFromNonNullTest(BoundExpression expression, ref LocalState sta
{
if (expression is BoundValuePlaceholderBase placeholder)
{
if (_resultForPlaceholdersOpt != null && _resultForPlaceholdersOpt.TryGetValue(placeholder, out var value))
if (PlaceholderMustBeRegistered(placeholder))
Copy link
Contributor

@AlekseyTs AlekseyTs Dec 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

PlaceholderMustBeRegistered

I am not sure if it is worth crashing compiler if the requirement is violated and duplicating the logic to achieve that. It feels like a simple assert within the original else branch would be sufficient. #Closed

AlekseyTs
AlekseyTs reviewed Dec 1, 2021
View reviewed changes
src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs Outdated
VisitPlaceholderWithReplacement(node);
return null;
}

private void VisitPlaceholderWithReplacement(BoundValuePlaceholderBase node)
{
if (PlaceholderMustBeRegistered(node))
Copy link
Contributor

@AlekseyTs AlekseyTs Dec 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

PlaceholderMustBeRegistered

I am not sure if it is worth crashing compiler if the requirement is violated and duplicating the logic to achieve that. It feels like a simple assert within the original else branch would be sufficient. #Closed

AlekseyTs
AlekseyTs approved these changes Dec 1, 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 1)

@jcouv
Copy link
Member Author

jcouv commented Dec 1, 2021

@dotnet/roslyn-compiler for second review. Small PR. Thanks

AlekseyTs
AlekseyTs reviewed Dec 1, 2021
View reviewed changes
src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs Outdated
@@ -4177,9 +4218,17 @@ private void LearnFromNonNullTest(BoundExpression expression, ref LocalState sta
{
if (expression is BoundValuePlaceholderBase placeholder)
{
if (_resultForPlaceholdersOpt != null && _resultForPlaceholdersOpt.TryGetValue(placeholder, out var value))
if (_resultForPlaceholdersOpt != null)
Copy link
Contributor

@AlekseyTs AlekseyTs Dec 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

_resultForPlaceholdersOpt != null

Could you elaborate why the condition got split? Don't we want to assert if the dictionary is null? #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.

Just me being stupid... :-P

AlekseyTs
AlekseyTs reviewed Dec 1, 2021
View reviewed changes
src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs Outdated
SetResult(node, result.RValueType, result.LValueType);
}
else
if (_resultForPlaceholdersOpt != null)
Copy link
Contributor

@AlekseyTs AlekseyTs Dec 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

_resultForPlaceholdersOpt != null

Same comment as for LearnFromNonNullTest #Closed

cston
cston reviewed Dec 1, 2021
View reviewed changes
src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs Outdated
Debug.Assert(removed);
}

private bool PlaceholderMustBeRegistered(BoundValuePlaceholderBase placeholder)
Copy link
Member

Choose a reason for hiding this comment

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

private

static

@jcouv jcouv enabled auto-merge (squash) December 2, 2021 03:34
This was referenced Dec 2, 2021
Open
Open
333fred
333fred reviewed Dec 2, 2021
View reviewed changes
src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs Outdated
Debug.Assert(removed);
}

private static bool PlaceholderMustBeRegistered(BoundValuePlaceholderBase placeholder)
Copy link
Member

Choose a reason for hiding this comment

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

This appears to only be used as an assert. Consider making this a conditional method and doing the assert directly.

@runfoapp runfoapp bot mentioned this pull request Dec 3, 2021
Closed
AlekseyTs
AlekseyTs reviewed Dec 3, 2021
View reviewed changes
src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs
default:
// Newer placeholders are expected to follow placeholder discipline, namely that
// they must be added to the map before they are visited, then removed.
throw ExceptionUtilities.UnexpectedValue(placeholder.Kind);
Copy link
Contributor

Choose a reason for hiding this comment

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

throw

Did you really want to throw? Calling ExceptionUtilities.UnexpectedValue(placeholder.Kind) should be sufficient, it will assert.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't see a problem with throwing as we do everywhere else. It's clear.

@jcouv jcouv disabled auto-merge December 4, 2021 01:13
@jcouv jcouv enabled auto-merge (squash) December 4, 2021 01:15
@jcouv
Copy link
Member Author

jcouv commented Dec 4, 2021

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 4 pipeline(s).

@jcouv jcouv merged commit 1741824 into dotnet:main Dec 4, 2021
@ghost ghost added this to the Next milestone Dec 4, 2021
This was referenced Dec 6, 2021
Closed
Closed
333fred added a commit to 333fred/roslyn that referenced this pull request Dec 6, 2021
@333fred
Merge remote-tracking branch 'upstream/main' into interp-nullable-imp…
d20c75f 
…rovements

* upstream/main: (68 commits)
  Lazy load ISourceLinkService to reduce DLL loads (dotnet#58108)
  [main] Update dependencies from dotnet/source-build (dotnet#57707)
  [main] Update dependencies from dotnet/arcade (dotnet#57968)
  Factor nullability logic for placeholders (dotnet#58036)
  Standardize list pattern lowering on `Index` constructor. (dotnet#58055)
  Add scripts to verify if a branch is ready to review
  Merge pull request dotnet#58100 from dotnet/dev/jorobich/skip-test
  Fix some places we weren't correctly disposing of VisualStudioAnalyzers
  Fix analyzer references being removed and added in one batch
  Fix indenting
  Ensure we don't silently capture any exceptions
  Don't MEF import the implementation directly if the public type will do
  Change comment
  Add comment
  Use actual jump tables
  Remove unused function
  Revert
  Simplify code
  Compute kind on demand
  Reorder
  ...
@jcouv jcouv modified the milestones: Next, 17.1 Dec 7, 2021
@jcouv jcouv deleted the nullable-placeholders branch December 7, 2021 05:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AlekseyTs AlekseyTs AlekseyTs approved these changes

@333fred 333fred 333fred left review comments

@cston cston cston approved these changes

Assignees

@jcouv jcouv

Labels
Area-Compilers
Projects
None yet
Milestone
17.1
Development

Successfully merging this pull request may close these issues.
4 participants
@jcouv @333fred @AlekseyTs @cston