Order GlobalNamespace declarations to match Compilation.SyntaxTrees #11625
Conversation
|
@dotnet/roslyn-compiler, @mavasani please review. |
|
Meets the bar, pending the usual criteria. |
| @@ -2934,7 +2934,8 @@ public IEnumerable<ISymbol> GetSymbolsWithName(Func<string, bool> predicate, Sym | |||
| var result = new HashSet<ISymbol>(); | |||
| var spine = new List<MergedNamespaceOrTypeDeclaration>(); | |||
|
|
|||
| AppendSymbolsWithName(spine, _compilation.Declarations.MergedRoot, predicate, filter, result, cancellationToken); | |||
| var mergedRoot = ((SourceNamespaceSymbol)_compilation.SourceModule.GlobalNamespace).MergedDeclaration; | |||
There was a problem hiding this comment.
Should we be concerned with the fact that now we are always creating SourceModuleSymbol here, which means that references are getting resolved etc., whereas before, if result is empty, it looks like we could have avoided doing this?
There was a problem hiding this comment.
Definitely interested in knowing the answer to this. In particular the potential impact this could have to performance.
|
It feels like this change is decreasing our ability to take advantage of caching done in Declaration Table across different compilation versions when the set and the order of syntax trees is the same. It looks like for the purpose of creating symbols, each compilation would always create its own merged root of namespace declarations, regardless whether the order differs or not. In addition, some code still uses MergedRoot from the declaration table, so we are somewhere in between, which potentially could be a source of inconsistencies and again prevents us from taking full advantage of the caching (we are splitting between two caches). We still have a risk of someone using the “wrong” merged root in future code, introducing hard to detect bugs. BTW, the test failures look like something that could be related to the change. |
|
Responding to @tmat's comment in the other PR, no concern about |
cston
force-pushed
the
11015-s
branch
2 times, most recently
from
a523fcb to
5ebaded
Compare
|
@AlekseyTs good catch regarding the caching. I've moved the caching back to the |
| builder.AddRange(oldRootDeclarations); | ||
| builder.Add(_latestLazyRootDeclaration.Value); | ||
| // Sort the root namespace declarations to match the order of SyntaxTrees. | ||
| if (compilation != null) |
There was a problem hiding this comment.
Do we expect compilation ever be null?
There was a problem hiding this comment.
compilation is null in DeclarationTests.
|
LGTM, modulo the test failures. I assume baseline for some tests should be adjusted? |
|
@AlekseyTs handling the test failures required a fix in |
|
@dotnet/roslyn-compiler please provide a second review, thanks. |
|
The current revision LGTM. |
|
LGTM |
The order of the root namespace declarations in
DeclarationTableno longer matches the order ofCompilation.SyntaxTreesafter callingCompilation.ReplaceSyntaxTree, andReplaceSyntaxTreeis called by the IDE when opening or updating a document.Fixes #11015.