(rebased) Add 'annotations' and 'warnings' support to nullable directive#36464
(rebased) Add 'annotations' and 'warnings' support to nullable directive#36464RikkiGibson merged 19 commits intodotnet:masterfrom
Conversation
src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs
Outdated
Show resolved
Hide resolved
| } | ||
| } | ||
|
|
||
| private static IEnumerable<string> ParseWarnAsErrorWarnings(string value) |
There was a problem hiding this comment.
ParseWarnAsErrorWarnings [](start = 43, length = 24)
This raises an interesting question: should we allow warnAsError:nullable the same way we allow noWarn:nullable.
@AlekseyTs, @gafter, @cston, what do you think? #Closed
There was a problem hiding this comment.
Aleksey also thought supporting warnAsError:nullable would be good and consistent.
Could you factor the code back together?
In reply to: 294552006 [](ancestors = 294552006)
There was a problem hiding this comment.
src/Features/CSharp/Portable/Completion/KeywordRecommenders/AnnotationsKeywordRecommender.cs
Show resolved
Hide resolved
| var position = location.SourceSpan.Start; | ||
|
|
||
| bool isNullableFlowAnalysisWarning = ErrorFacts.NullableFlowAnalysisWarnings.Contains(id); | ||
| if (isNullableFlowAnalysisWarning) |
There was a problem hiding this comment.
if (isNullableFlowAnalysisWarning) [](start = 12, length = 34)
I suspect this fixes #32742
nit: I'm tempted to move this block (checking the nullability "warnings" flag) at the top of this method (before step 1). #Closed
There was a problem hiding this comment.
What do you think of moving the isNullableFlowAnalysisWarning handling at the start of this method?
As it stands, we're computing report above this block, but we're checking it below this block, which is weird.
In reply to: 294555110 [](ancestors = 294555110)
There was a problem hiding this comment.
I think you're right, it might as well occur before step 1.
In reply to: 295034304 [](ancestors = 295034304,294555110)
There was a problem hiding this comment.
Could you add some tests to confirm whether you've fixed #32742 with this PR?
One scenario to start:
#nullable enable annotations
class Base { virtual void M(List<string> s) { } }
class C : Base { override void M(List<string?> s) { } } // expecting no nullability warning since nullable warnings context is false, even though the relevant OHI warning (CS8610) isn't produced by NullableWalkerIn reply to: 295034304 [](ancestors = 295034304,294555110)
There was a problem hiding this comment.
I'm adding a few tests however I believe this isn't resolved by this PR because it's simply implemented by checking the ImmutableHashSet<ErrorCode> NullableFlowAnalysisWarnings.
In reply to: 295043412 [](ancestors = 295043412,295034304,294555110)
src/Compilers/CSharp/Portable/Compilation/CSharpDiagnosticFilter.cs
Outdated
Show resolved
Hide resolved
| @@ -41,13 +53,15 @@ private NullableDirectiveMap(ImmutableArray<(int Position, bool? State)> directi | |||
| } | |||
|
|
|||
| /// <summary> | |||
There was a problem hiding this comment.
summary [](start = 13, length = 7)
This doc should probably be moved to NullableDirective now. #Closed
| @@ -624,7 +624,7 @@ internal PragmaWarningState GetPragmaDirectiveWarningState(string id, int positi | |||
| /// `enable` or `safeonly`, false if `disable`, and null if no preceding directive, | |||
There was a problem hiding this comment.
safeonly [](start = 25, length = 8)
Could you fix or remove this comment as well? safeonly is no longer an option. Thanks #Closed
Marking 'wontfix', only to mean that I don't intend to act on this in this PR. But it might make sense in a future PR. In reply to: 502893961 [](ancestors = 502893961) Refers to: src/Compilers/CSharp/Portable/Syntax/Syntax.xml:4437 in 27861ef. [](commit_id = 27861ef, deletion_comment = False) |
jcouv
left a comment
There was a problem hiding this comment.
Done with review pass (iteration 14).
I'd recommend you use CodeFlow to ensure you don't miss some feedback.
|
reviewing now #Closed |
| CompileAndVerify(comp3, symbolValidator: | ||
| (ModuleSymbol m) => | ||
| { | ||
| (string type, string attribute)[] baseline = new[] |
There was a problem hiding this comment.
new[] [](start = 65, length = 5)
could be factored out #Closed
| ("System.Collections.Generic.Dictionary<System.Object?, System.Int32>?", "System.Runtime.CompilerServices.NullableAttribute({2, 2, 0})") | ||
| }; | ||
|
|
||
| var comp3 = CreateCompilation(new[] { source }, options: WithNonNullTypes(NullableContextOptions.Annotations)); |
There was a problem hiding this comment.
CreateCompilation(new[] { source } [](start = 28, length = 34)
Consider also testing with directive by testing CreateCompilation("#nullable enable annotations /*newline*/" + source) #Closed
| ushort number; | ||
| if (ushort.TryParse(id, NumberStyles.Integer, CultureInfo.InvariantCulture, out number) && | ||
| ErrorFacts.IsWarning((ErrorCode)number)) | ||
| if (id.ToLowerInvariant() == "nullable") |
There was a problem hiding this comment.
if (id.ToLowerInvariant() == "nullable") [](start = 16, length = 40)
Probably should use if (string.Equals(accessibility, "nullable", StringComparison.OrdinalIgnoreCase)) ... as the rest of this file does #Closed
There was a problem hiding this comment.
Yes, that should avoid allocating an extra string, and Invariant is the wrong comparer for strings. #Closed
| { | ||
| var builder = ArrayBuilder<(int Position, bool? State)>.GetInstance(); | ||
| // Generated files have an initial nullable context that is "disabled" | ||
| var previousDirective = isGeneratedCode |
There was a problem hiding this comment.
var previousDirective = isGeneratedCode [](start = 12, length = 39)
nit: consider factoring this and similar snippet above (line 73) out to a method GetDirectiveForFileStart(bool isGeneratedCode) #Closed
There was a problem hiding this comment.
Marking this closed because I assume your approval includes approval for this change
In reply to: 295073254 [](ancestors = 295073254)
jcouv
left a comment
There was a problem hiding this comment.
Done with review pass (iteration 15)
| ushort number; | ||
| if (ushort.TryParse(id, NumberStyles.Integer, CultureInfo.InvariantCulture, out number) && | ||
| ErrorFacts.IsWarning((ErrorCode)number)) | ||
| if (id.ToLowerInvariant() == "nullable") |
There was a problem hiding this comment.
Yes, that should avoid allocating an extra string, and Invariant is the wrong comparer for strings. #Closed
| /// where <see langword="true"/> means the context is 'enable', <see langword="false"/> means the context is 'disable', | ||
| /// and <see langword="null"/> means the context is 'restore' or not specified. | ||
| /// </summary> | ||
| internal readonly struct NullableDirective |
There was a problem hiding this comment.
I would call this something else, because it doesn't really represent a NullableDirective. Maybe NullableContextState? #Resolved
There was a problem hiding this comment.
OK, using PragmaWarningStateMap as a precedent, how about we change the name NullableDirective to NullableContextState, and change NullableDirectiveMap to NullableContextStateMap. #Resolved
| var builder = ArrayBuilder<(int Position, bool? State)>.GetInstance(); | ||
| // Generated files have an initial nullable context that is "disabled" | ||
| var previousDirective = isGeneratedCode | ||
| ? new NullableDirective(0, false, false) |
There was a problem hiding this comment.
Named arguments for these literals? #Resolved
|
@RikkiGibson Did this need any IDE signoff? |
|
It adds some pretty rudimentary keyword recommenders and classifier changes, it didn't seem necessary to have a dedicated IDE review. |
|
@jasonmalinowski Sorry about that. We should have waited for an IDE sign-off. |
|
@jcouv @RikkiGibson I agree that in this case it probably didn't, but sometimes we'll still see what you have done and decide if we need to add additional tests or file additional bugs for additional work. |
* dotnet/master: (85 commits) Don't complete statement when typing semicolon inside comments in an argument list (dotnet#36521) Set focus to editor before finding text Add a bunch of nullability support to some code generation helpers Add 'annotations' and 'warnings' support to nullable directive (dotnet#36464) Fixed IDE services touching `notnull` constraint (dotnet#36508) Update compiler toolset to arcade version (dotnet#36549) Fix for 923157 Do not include value types in NullableAttribute byte[] (dotnet#36519) Fix a deadlock in InvokeOnUIThread Apply a hang mitigating timeout to UI thread operations Move to a different lowering from for nullable value types to work around a bug in TransformCompoundAssignmentLHS. Addressed PR feedback. Fix stack overflow in requesting syntax directives (dotnet#36347) crash on ClassifyUpdate for EventFields (dotnet#35962) fixing bad merge with refactoring that was checked in later added basic completion statement telemetry Remove duplication in AbstractSymbolCompletionProvider.CreateItems Disable move type when the options service isn't present (dotnet#36334) Fix crash where type inference doing method inference needs to drop nullability Revert "Use IVsSolution to look up IVsHierarchy by project GUID (dotnet#35746)" Fix parsing bug in invalid using statements (dotnet#36428) ...
Resolves #35730. Resolves #35748. Resolves #35747.
You may wish to review this PR commit-by-commit, as several of the commits are just boilerplate to add or remove keywords from syntax nodes, etc.