Skip to content

Expose display option to add ! on non-nullable types #40519

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
Merged
merged 2 commits into from
Jan 4, 2020
Merged

Expose display option to add ! on non-nullable types #40519

merged 2 commits into from
Jan 4, 2020

Conversation

@jcouv
Copy link
Member

@jcouv jcouv commented Dec 20, 2019

To display ! in shipped API files (WIP PR at dotnet/roslyn-analyzers#3125), we'll need to expose the previously-internal display option.

@jcouv jcouv added this to the 16.5 milestone Dec 20, 2019
@jcouv jcouv self-assigned this Dec 20, 2019
@jcouv jcouv mentioned this pull request Dec 20, 2019
Merged
@jcouv jcouv force-pushed the non-nullable-option branch from e227aa5 to c902e82 Compare December 20, 2019 01:11
@jcouv jcouv marked this pull request as ready for review December 20, 2019 01:47
@jcouv jcouv requested a review from a team as a code owner December 20, 2019 01:47
@jcouv jcouv modified the milestones: 16.5, 16.5.P2 Dec 20, 2019
@sharwell
Copy link
Contributor

sharwell commented Dec 20, 2019 •
edited
Loading

There is no need to expose this flag for PublicApiAnalyzers. Regardless of whether this flag is internal or public, the analyzer will be using the flag only via reflection. If you want to expose it for other reasons, then that's fine too.

@jcouv
Copy link
Member Author

jcouv commented Dec 26, 2019

the analyzer will be using the flag only via reflection.

What makes you say that? The analyzer for shipped APIs makes a SymbolDisplayFormat from the published API, not using reflection as far as I can tell.

https://github.com/dotnet/roslyn-analyzers/blob/master/src/PublicApiAnalyzers/Core/Analyzers/DeclarePublicApiAnalyzer.cs#L114-L134

@sharwell
Copy link
Contributor

sharwell commented Dec 26, 2019

@jcouv the analyzer references an old version of the compiler package, so unless the new flag is back-ported to Roslyn 1.x, it would only be accessible via light-up (reflection).

@jcouv
Copy link
Member Author

jcouv commented Dec 27, 2019

@sharwell Reflection is not the only way to use the new flag. You can cast a number to an enum type. That's what I did in WIP PR.

private const int IncludeNullableReferenceTypeModifier = 1 << 6;

...
                    SymbolDisplayMiscellaneousOptions.UseSpecialTypes |
                    (SymbolDisplayMiscellaneousOptions)IncludeNullableReferenceTypeModifier); // cast of constant to enum type

This hard-coding is safe because the enum value is part of public API (ie. won't change).

@jcouv
Copy link
Member Author

jcouv commented Dec 30, 2019

@dotnet/roslyn-compiler for review. This PR will unblock an analyzer PR (to include nullability information in shipped APIs).
The updated analyzer will be able to pass this value even though the analyzer will not take a dependency on new compiler bits, by casting a hard-coded constant to enum type. See WIP PR for the analyzer.

@jcouv jcouv added the Feature - Nullable Reference Types Nullable Reference Types label Dec 30, 2019
@jcouv
Copy link
Member Author

jcouv commented Jan 2, 2020

@333fred @cston Would you have time to review this simple change? That will unblock another PR I'm working on (in roslyn-analyzers). Thanks

cston
cston reviewed Jan 3, 2020
View reviewed changes
src/Compilers/CSharp/Portable/SymbolDisplay/SymbolDisplayVisitor.Types.cs Outdated
case CodeAnalysis.NullableAnnotation.Annotated:
if (format.MiscellaneousOptions.IncludesOption(SymbolDisplayMiscellaneousOptions.IncludeNullableReferenceTypeModifier) &&
if ((format.MiscellaneousOptions.IncludesOption(SymbolDisplayMiscellaneousOptions.IncludeNullableReferenceTypeModifier) ||
format.MiscellaneousOptions.IncludesOption(SymbolDisplayMiscellaneousOptions.IncludeNonNullableReferenceTypeModifier)) &&
Copy link
Contributor

@cston cston Jan 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Is this change needed? (It seems strange that IncludeNonNullable... implies IncludeNullable... but not the reverse. Can we separate the two completely?) #Closed

Copy link
Member Author

@jcouv jcouv Jan 3, 2020

Choose a reason for hiding this comment

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

I'd be okay with making the two flags independent, but I couldn't think of a scenario when we would want to print ! annotations but not ? annotations.

Copy link
Member Author

@jcouv jcouv Jan 3, 2020

Choose a reason for hiding this comment

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

@333fred What's your thought on the relationship of these two flags?

Previously, we asserted that we only use the ! flag when we also use the ? flag. But an assertion isn't sufficient for public APIs, obviously. We could make the flag independent, or we could throw an exception when an invalid combination of flags is produced.

333fred
333fred approved these changes Jan 3, 2020
View reviewed changes
Copy link
Member

@333fred 333fred left a comment

Choose a reason for hiding this comment

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

:shipit:

@jcouv
Copy link
Member Author

jcouv commented Jan 3, 2020

Dankeshon

I'm re-running the integration branch. It has been consistently flaky in multiple PRs for me recently. Has anyone else run into this?

@jcouv jcouv merged commit c18811b into dotnet:master Jan 4, 2020
@jcouv jcouv deleted the non-nullable-option branch January 4, 2020 01:07
333fred added a commit to 333fred/roslyn that referenced this pull request Jan 8, 2020
@333fred
Merge remote-tracking branch 'dotnet/master' into merge-master
9e13c01 
* dotnet/master: (592 commits)
  Improve nullable analysis of local functions (dotnet#40422)
  Fix race condition in CodeFixService
  Annotate CSharpCompilation (dotnet#40752)
  Revert "Merge pull request dotnet#40765 from dibarbet/revert_use_index"
  More feedback
  address feedback
  More refactoring and hardening of remote calls (dotnet#40395)
  Update PublishData.json
  Unskip and fix integration test
  Update configs for preview 2 snap
  Improve error message for CS0191 (dotnet#40748)
  Revert "Merge pull request dotnet#40410 from sharwell/use-index"
  PR feedback
  Report erroneous implicit conversions in a switch expression (dotnet#40678)
  Ignore dynamic vs object, etc in pattern-matching machinery. (dotnet#40677)
  Handle dependent slots in pattern-matching null tests. (dotnet#39625)
  Pass non-null arguments to avoid nullability warning
  Update dependencies from https://github.com/dotnet/arcade build 20200104.1 (dotnet#40749)
  [master] Update dependencies from dotnet/arcade (dotnet#40718)
  Expose display option to add ! on non-nullable types (dotnet#40519)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@333fred 333fred 333fred approved these changes

+1 more reviewer

@cston cston cston approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@jcouv jcouv

Labels

Area-Compilers Feature - Nullable Reference Types Nullable Reference Types

Projects

None yet

Milestone

16.5.P2

Development

Successfully merging this pull request may close these issues.

4 participants

@jcouv @sharwell @333fred @cston