Annotate more CodeAnalysis and CodeAnalysis.CSharp public types by jcouv · Pull Request #43025 · dotnet/roslyn

jcouv · 2020-04-02T17:58:39Z

No description provided.

jcouv · 2020-04-08T16:24:24Z

@dotnet/roslyn-compiler for review. Thanks #Resolved

333fred · 2020-04-08T21:37:13Z

src/Compilers/Core/Portable/Syntax/SyntaxNodeExtensions.cs

        /// <summary>
        /// Attaches the node to a SyntaxTree that the same options as <paramref name="oldTree"/>
        /// </summary>
+        [return:NotNullIfNotNull("node")]


Should this imply that node should be annotated? #Closed

333fred · 2020-04-08T21:39:04Z

src/Compilers/CSharp/Portable/Syntax/CSharpSyntaxNode.cs

                {
                    if (child.IsNode)
                    {
-                        var d = child.AsNode().GetLastDirective(predicate);


Do you think it's worth adding an extension method for this pattern with an out? #Resolved

Debated it, but Jared didn't introduce such pattern for most syntax changes he made. I'll stay with existing pattern.

In reply to: 405830860 [](ancestors = 405830860)

I definitely didn't use that pattern when I was first exploring annotating syntax. I was more getting used to the feature and didn't wan to change as much around it while I was annotating. Now though I'm more comfortable with that type of change and if we have a handy helper would opt for using it. #Resolved

We don't have a helper yet. Would rather keep that separate.

In reply to: 414137782 [](ancestors = 414137782)

Oh, actually, we do have AsNode(out ...). I'll use that

In reply to: 414179069 [](ancestors = 414179069,414137782)

333fred · 2020-04-08T21:51:14Z

src/Compilers/Core/Portable/DocumentationComments/DocumentationProvider.cs

        /// <param name="cancellationToken">A cancellation token for the search.</param>
        /// <returns>A DocumentationComment.</returns>
-        protected internal abstract string GetDocumentationForSymbol(
+        protected internal abstract string? GetDocumentationForSymbol(


string? [](start = 36, length = 7)

Man, this API is a real victim of lack of NRT. Some implementations return string.Empty when nothing is there, some return null. #WontFix

333fred · 2020-04-08T21:57:16Z

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalysisResultBuilder.cs

            lazyLocalDiagnostics = lazyLocalDiagnostics ?? new Dictionary<SyntaxTree, Dictionary<DiagnosticAnalyzer, ImmutableArray<Diagnostic>.Builder>>();

-            foreach (var diagsByTree in diagnostics.GroupBy(d => d.Location.SourceTree))
+            foreach (var diagsByTree in diagnostics.GroupBy(d => d.Location.SourceTree!))


Why is this safe? #Closed

From discussion with @mavasani, only diagnostics that are in source get into the local queues, others are treated as non-local or compilation.
Last commit (15) adds assertions instead of this suppression.

In reply to: 405838845 [](ancestors = 405838845)

333fred · 2020-04-08T22:00:13Z

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalysisValueProvider.cs

        private WrappedValue ComputeValue(TKey key)
        {
            var value = _computeValue(key);
-            return new WrappedValue { Value = value };


First time I've seen us fall victim to this pattern. #Closed

333fred · 2020-04-08T22:11:51Z

src/Compilers/CSharp/Portable/Syntax/SyntaxExtensions.cs


        internal static XmlNameAttributeElementKind GetElementKind(this XmlNameAttributeSyntax attributeSyntax)
        {
+            RoslynDebug.Assert(attributeSyntax.Parent is object);


Shouldn't be necessary. #Closed

Will switch to to Debug.Assert (assuming that's what you meant)

In reply to: 405844872 [](ancestors = 405844872)

333fred · 2020-04-08T22:12:14Z

Done review pass (commit 13) #Resolved

jcouv · 2020-04-22T22:52:09Z

@333fred @dotnet/roslyn-compiler Please take another look. Thanks #Resolved

333fred

LGTM (commit 15)

333fred · 2020-04-23T20:47:45Z

Looks like there's a newly introduced warning though. #Resolved

jcouv · 2020-04-23T20:54:06Z

Yeah, I'll merge with latest master and resolve, but only after the review as some tools (cough CodeFlow) don't handle merge commits well ;-) #Resolved

jaredpar · 2020-04-23T21:26:40Z

src/Compilers/CSharp/Portable/Lowering/Instrumentation/DebugInfoInjector.cs

                    case SyntaxKind.BaseConstructorInitializer:
                    case SyntaxKind.ThisConstructorInitializer:
                        var init = (ConstructorInitializerSyntax)original.Syntax;
+                        Debug.Assert(init.Parent is object);


Confused why this would be necessary but the line above was able to cast without adding a ? to the type. If init was indeed maybe null here I would expect that the type cast above would have to say that as well. #Resolved

The problem is not that init is maybe-null, but that init.Parent is.

In reply to: 414135650 [](ancestors = 414135650)

jaredpar · 2020-04-23T21:27:39Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceFieldLikeEventSymbol.cs

            _name = declaratorSyntax.Identifier.ValueText;

            var declaratorDiagnostics = DiagnosticBag.GetInstance();
+            Debug.Assert(declaratorSyntax.Parent is object);


Consider moving this Debug.Assert to the top of the method body as it's effectively a pre-condition for the method itself, not this specific point in the method. #Resolved

jaredpar · 2020-04-23T21:29:26Z

src/Compilers/CSharp/Portable/Syntax/CSharpSyntaxNode.cs

            {
                if (child.ContainsDirectives)
                {
                    if (child.IsNode)


Consider using child.AsNode(out var node) here to avoid the ! below. #Resolved

jaredpar · 2020-04-23T21:30:48Z

src/Compilers/CSharp/Portable/Syntax/CSharpSyntaxNode.cs

                {
                    if (child.IsNode)
                    {
-                        var d = child.AsNode().GetLastDirective(predicate);


I definitely didn't use that pattern when I was first exploring annotating syntax. I was more getting used to the feature and didn't wan to change as much around it while I was annotating. Now though I'm more comfortable with that type of change and if we have a handy helper would opt for using it. #Resolved

jcouv added 11 commits April 2, 2020 10:51

Annotate CSharpSyntaxNode

14d9aaf

Annotate CSharpDiagnosticFormatter

f5212be

Annotate CSharpSyntaxRewriter

517f072

Annotate CSharpSyntaxVisitor

0b3f677

Annotate AnalyzerConfigOptionsProvider and DocumentationProvider

dd304c2

Annotate remainder of AssemblyIdentity

958d740

Annotate AnalysisResult

0e572b1

Annotate AnalysisResultBuilder

2b33b80

Annotate Suppression

29e28cf

Annotate AnalysisValueProvider

9172fb4

Tweak

328f6af

jcouv added Area-Compilers Concept-Null Annotations The issue involves annotating an API for nullable reference types labels Apr 2, 2020

jcouv self-assigned this Apr 2, 2020

jcouv changed the title ~~Annotate more CodeAnalysis and CodeAnalysis public types~~ Annotate more CodeAnalysis and CodeAnalysis.CSharp public types Apr 2, 2020

jcouv marked this pull request as ready for review April 2, 2020 18:32

jcouv requested review from a team as code owners April 2, 2020 18:32

jcouv added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Apr 2, 2020

jcouv added 2 commits April 2, 2020 13:39

More adjustments

1820fc0

More tweaks

b09c20a

jcouv removed the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Apr 3, 2020

jcouv added this to the 16.7 milestone Apr 3, 2020

333fred reviewed Apr 8, 2020

View reviewed changes

Julien Couvreur added 2 commits April 10, 2020 16:36

Address PR feedback

129447c

Add assertions in analyzer code

0871012

jcouv requested a review from 333fred April 22, 2020 22:51

333fred approved these changes Apr 23, 2020

View reviewed changes

jaredpar approved these changes Apr 23, 2020

View reviewed changes

Julien Couvreur added 5 commits April 23, 2020 16:04

Address feedback from Jared

56bd34a

Merge remote-tracking branch 'dotnet/master' into annotate-more5

08925da

Resolve merge conflict

f92e998

Merge remote-tracking branch 'dotnet/master' into annotate-more5

40e727b

Resolve conflict by re-generating compiler files

ba71790

jcouv merged commit 2a4b133 into dotnet:master Apr 24, 2020

ghost modified the milestones: 16.7, Next Apr 24, 2020

sharwell modified the milestones: Next, temp, 16.7.P1 Apr 28, 2020

Conversation

jcouv commented Apr 2, 2020

Uh oh!

jcouv commented Apr 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

333fred Apr 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

333fred Apr 8, 2020 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Apr 22, 2020

Choose a reason for hiding this comment

Uh oh!

jaredpar Apr 23, 2020 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Apr 23, 2020

Choose a reason for hiding this comment

Uh oh!

jcouv Apr 23, 2020

Choose a reason for hiding this comment

Uh oh!

333fred Apr 8, 2020 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

333fred Apr 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Apr 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

333fred Apr 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

333fred Apr 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Apr 10, 2020

Choose a reason for hiding this comment

Uh oh!

333fred commented Apr 8, 2020 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcouv commented Apr 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

333fred left a comment

Choose a reason for hiding this comment

Uh oh!

333fred commented Apr 23, 2020 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcouv commented Apr 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jaredpar Apr 23, 2020 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Apr 23, 2020

Choose a reason for hiding this comment

Uh oh!

jaredpar Apr 23, 2020 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaredpar Apr 23, 2020 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv commented Apr 8, 2020 •

edited

Loading

333fred Apr 8, 2020 •

edited

Loading

333fred Apr 8, 2020 •

edited by jcouv

Loading

jaredpar Apr 23, 2020 •

edited by jcouv

Loading

333fred Apr 8, 2020 •

edited by jcouv

Loading

333fred Apr 8, 2020 •

edited

Loading

jcouv Apr 22, 2020 •

edited

Loading

333fred Apr 8, 2020 •

edited

Loading

333fred Apr 8, 2020 •

edited

Loading

333fred commented Apr 8, 2020 •

edited by jcouv

Loading

jcouv commented Apr 22, 2020 •

edited

Loading

333fred commented Apr 23, 2020 •

edited by jcouv

Loading

jcouv commented Apr 23, 2020 •

edited

Loading

jaredpar Apr 23, 2020 •

edited by jcouv

Loading

jaredpar Apr 23, 2020 •

edited by jcouv

Loading

jaredpar Apr 23, 2020 •

edited by jcouv

Loading

jaredpar Apr 23, 2020 •

edited by jcouv

Loading