Skip to content

Add nullable ref annotations to SyntaxFactory#54199

Merged
jcouv merged 4 commits intomainfrom
dev/andarno/nullableReturnExpressions
Jun 25, 2021
Merged

Add nullable ref annotations to SyntaxFactory#54199
jcouv merged 4 commits intomainfrom
dev/andarno/nullableReturnExpressions

Conversation

@AArnott
Copy link
Contributor

@AArnott AArnott commented Jun 18, 2021

The overloads these were calling allow for null values.

There are probably many more like this that I didn't happen to run into. I recommend an audit around the pattern where these bugs were found.

The overloads these were calling allow for null values
@AArnott AArnott requested a review from a team as a code owner June 18, 2021 12:51
@ghost ghost added the Area-Compilers label Jun 18, 2021
@AArnott AArnott changed the title Add nullable ref annotation for ReturnStatement Add nullable ref annotations to SyntaxFactory Jun 18, 2021
@jcouv jcouv added the Concept-Null Annotations The issue involves annotating an API for nullable reference types label Jun 21, 2021
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 Thanks (iteration 4)

@jcouv jcouv self-assigned this Jun 24, 2021
@AArnott
Copy link
Contributor Author

AArnott commented Jun 25, 2021

@jcouv I'm not familiar with PR practices in this repo. Should I complete this PR at this point?

@jcouv
Copy link
Member

jcouv commented Jun 25, 2021

@AArnott Thanks for the ping. The general rule is that every compiler PR needs two reviews (including one senior). Then we merge/squash by default to simplify git history.

@dotnet/roslyn-compiler for a second review. Thanks

@RikkiGibson RikkiGibson self-assigned this Jun 25, 2021
@RikkiGibson
Copy link
Member

The issue makes me wonder if there is some heuristic we could use in the IDE to determine if a parameter's non-nullability is "unnecessary". For example, if we did an additional flow analysis where we assumed that the parameter may be null, and there were no new warnings produced.

@jcouv jcouv merged commit a282933 into main Jun 25, 2021
@ghost ghost added this to the Next milestone Jun 25, 2021
@jcouv jcouv deleted the dev/andarno/nullableReturnExpressions branch June 25, 2021 21:13
333fred added a commit that referenced this pull request Jun 25, 2021
…ures/caller-argument-expression

* upstream/main: (492 commits)
  Add nullable ref annotations to SyntaxFactory (#54199)
  Add 'replace' APIs and hook them up to the IDE (#54270)
  Allow implicit implementation of non-public interface members (#54182)
  Make insertion a stage of the official build (#54376)
  Cleanup
  Remove unused property
  Simplify glyph computation
  Report all-empty top level statements. (#54385)
  Calculate TypeParameterKind based on the container type (#54200)
  vert
  not roaming
  Split tests
  Multple matches
  Report as we get results
  Fixup tests
  Update tests
  Avoid thread dependency in VirtualMemoryNotificationListener constructor
  Fast progression search.
  Add LanguageVersion 10 (#54359)
  Sure, why not
  ...
@alrz
Copy link
Member

alrz commented Jun 26, 2021

there were no new warnings produced.

Shouldn't compiler expose such API e.g. through speculative semantic model, instead of having to fork the compilation and check diagnostics?

@RikkiGibson
Copy link
Member

RikkiGibson commented Jun 26, 2021

Shouldn't compiler expose such API e.g. through speculative semantic model, instead of having to fork the compilation and check diagnostics?

I was thinking of the DataFlowsOut API as a precedent, where IIRC we do an extra definite assignment pass, treating assignments within the given region as unassignments, then if additional diagnostics are given as a result of those unassignments, we say that the affected variables "flow out" of the region. The diagnostics themselves aren't actually surfaced.

@RikkiGibson RikkiGibson modified the milestones: Next, 17.0.P2 Jun 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-Compilers Concept-Null Annotations The issue involves annotating an API for nullable reference types

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants