Allow diagnostics to be stored in the DeclarationTree.

[CyrusNajmabadi]

No description provided.

[CyrusNajmabadi]

As i have continued my work pulling our grammar-checks from the parser, i have continually run into a small snag that has made the work slightly more difficult than desirable.

Specifically, there are some paths in the compiler where we work hard to avoid going back to the syntax tree. While we are fine going back to the syntax tree so as to bind statements or expressions, and while we are fine going back to the syntax tree to make symbols for members, we try to avoid going back to the syntax for NamedTypeSymbols or NamespaceSymbols. Instead, we just make Declaration nodes in the Declaration tree and we make the Symbols out of the Declarations alone. This is problematic as there is not an existing opportunity to then check the syntax directly.

I previously dealt with this by storing flags in the declaration node indicating syntax information. We could then use that to decide if we should go back to the syntax in order to report the error. This is not a great approach as it requires double checking. We have to check while making the DeclTree in order to set the flags, then we need to check the flags, and go back to the syntax to check it and report the diagnostics.

I'm trying a new approach here where we simply store diagnostics if we have them in the decl-tree directly. This allows us to only check the syntax once and not force us to go back to it later on. It also provides a uniform way for us to move grammar errors out of the parser for namespaces/types. We can now move them to the decl-tree phase, and then add those diagnostics to the symbols later on when we create them.

The downside of this approach is the extra field used in the declarations. However, this is just an extra field per declaration. There are not actually that many declarations in a compilation. I measured performance of creating the declaration tree and never measured any actual CPU cost to the change. And even the compilation for CSharpCodeAnalysis only contains <3k declarations. So there is only a tiny increase in actual memory usage.

[CyrusNajmabadi]

Other grammar checks i want to move out of hte parser include:

1. checking the names of things like namepaces.
2. checking modifiers.

I was doing #2, but had no problem until i ran into TypeDeclarations. Because we don't want to go back to syntax we had no good place to check the modifiers. This wasn't a problem for things like member declarations as we do go back to their syntax. With this PR's approach it is now trivial to check the modifiers while creating the decl-tree.

[CyrusNajmabadi]

Tagging @dotnet/roslyn-compiler

[jcouv]

    internal enum TypeDeclarationFlags : byte

Does that mean these other flags will also eventually be removed too?

---

Refers to: src/Compilers/CSharp/Portable/Declarations/SingleTypeDeclaration.cs:21 in dfcf4d5. [](commit_id = dfcf4d5, deletion_comment = False)

[jcouv]

LGTM

[CyrusNajmabadi]

Does that mean these other flags will also eventually be removed too?

Not necessarily. The flag i removed was specifically added (by me) so we could go back to syntax to report an error if necessary. The other flags may not serve that purpose. They may be needed to go back to do things like binding and determining other information. In that case, they would not be removable.

[CyrusNajmabadi]

Tagging @dotnet/roslyn-compiler Can i get another set of eyes.

[CyrusNajmabadi]

@gafter Can you take a look?

[gafter]

Looking now.

[gafter]

LGTM

[CyrusNajmabadi]

@ManishJayaswal

Customer scenario

None. This is general infrastructure work for the compiler based on design changes the team agreed on.

Bugs this fixes:

N/A

Workarounds, if any

N/A

Risk

Low.

Performance impact

Low.

Is this a regression from a previous update?

N/A

Root cause analysis:

N/A.

How was the bug found?

N/A

[CyrusNajmabadi]

Rebased to 15.6