dotnet / roslyn

The Roslyn .NET compiler provides C# and Visual Basic languages with rich code analysis APIs.
https://docs.microsoft.com/dotnet/csharp/roslyn-sdk/
MIT License
19.13k stars 4.04k forks source link

EnC: Avoid completion of non-updated symbols #70212

Open tmat opened 1 year ago

tmat commented 1 year ago

When we emit EnC deltas the IDE passes a set of updated symbols to the compiler. Then CompileMethod is only called on those symbols.

However, GetSourceDeclarationDiagnostics called before that forces completion of the entire AssemblySymbol. Could only the symbols that were updated be completed?

From customer trace: image

>   Microsoft.CodeAnalysis.CSharp.dll!Microsoft.CodeAnalysis.CSharp.CSharpCompilation.GetSourceDeclarationDiagnostics(Microsoft.CodeAnalysis.SyntaxTree syntaxTree, Microsoft.CodeAnalysis.Text.TextSpan? filterSpanWithinTree, System.Func<System.Collections.Generic.IEnumerable<Microsoft.CodeAnalysis.Diagnostic>, Microsoft.CodeAnalysis.SyntaxTree, Microsoft.CodeAnalysis.Text.TextSpan?, System.Collections.Generic.IEnumerable<Microsoft.CodeAnalysis.Diagnostic>> locationFilterOpt, System.Threading.CancellationToken cancellationToken) Line 3108  C#
    Microsoft.CodeAnalysis.CSharp.dll!Microsoft.CodeAnalysis.CSharp.CSharpCompilation.GetDiagnosticsWithoutFiltering(Microsoft.CodeAnalysis.CompilationStage stage, bool includeEarlierStages, Microsoft.CodeAnalysis.CSharp.BindingDiagnosticBag builder, System.Threading.CancellationToken cancellationToken) Line 2893  C#
    Microsoft.CodeAnalysis.CSharp.dll!Microsoft.CodeAnalysis.CSharp.CSharpCompilation.GetDiagnostics(Microsoft.CodeAnalysis.CompilationStage stage, bool includeEarlierStages, Microsoft.CodeAnalysis.DiagnosticBag diagnostics, System.Threading.CancellationToken cancellationToken) Line 2818    C#
    Microsoft.CodeAnalysis.CSharp.dll!Microsoft.CodeAnalysis.CSharp.CSharpCompilation.GetDiagnostics(Microsoft.CodeAnalysis.CompilationStage stage, bool includeEarlierStages, System.Threading.CancellationToken cancellationToken) Line 2809  C#
    Microsoft.CodeAnalysis.CSharp.dll!Microsoft.CodeAnalysis.CSharp.CSharpCompilation.CompileMethods(Microsoft.CodeAnalysis.Emit.CommonPEModuleBuilder moduleBuilder, bool emittingPdb, Microsoft.CodeAnalysis.DiagnosticBag diagnostics, System.Predicate<Microsoft.CodeAnalysis.Symbols.ISymbolInternal> filterOpt, System.Threading.CancellationToken cancellationToken) Line 3360   C#
    Microsoft.CodeAnalysis.dll!Microsoft.CodeAnalysis.Compilation.Compile(Microsoft.CodeAnalysis.Emit.CommonPEModuleBuilder moduleBuilder, bool emittingPdb, Microsoft.CodeAnalysis.DiagnosticBag diagnostics, System.Predicate<Microsoft.CodeAnalysis.Symbols.ISymbolInternal> filterOpt, System.Threading.CancellationToken cancellationToken) Line 2590  C#
    Microsoft.CodeAnalysis.CSharp.dll!Microsoft.CodeAnalysis.CSharp.Emit.EmitHelpers.EmitDifference(Microsoft.CodeAnalysis.CSharp.CSharpCompilation compilation, Microsoft.CodeAnalysis.Emit.EmitBaseline baseline, System.Collections.Generic.IEnumerable<Microsoft.CodeAnalysis.Emit.SemanticEdit> edits, System.Func<Microsoft.CodeAnalysis.ISymbol, bool> isAddedSymbol, System.IO.Stream metadataStream, System.IO.Stream ilStream, System.IO.Stream pdbStream, Microsoft.CodeAnalysis.CodeGen.CompilationTestData testData, System.Threading.CancellationToken cancellationToken) Line 80 C#

We should also skip CLS compliance analysis when compiling deltas.

jaredpar commented 1 year ago

@cston to comment

We should also skip CLS compliance analysis when compiling deltas.

That seems reasonable.

jaredpar commented 1 year ago

@333fred how feasible do you think this is?

333fred commented 1 year ago

Seems rather infeasible to me. How could we know if an updated symbol invalidates some other usage without compiling the whole program? Unless we can guarantee that no API surface area changed, there will always be a case (sometimes reasonable, sometimes pathological) where the updates could break something that wasn't updated.

tmat commented 1 year ago

We already allow applying changes that are correct locally but do not compile globally. I.e. only the updated symbols need to be correct, other symbols might have errors. For example, you can change a signature of any method (including public) while keeping the callers unchanged.

As long as we can emit correct IL/metadata for the affected symbols we should not need to validate other symbols.

333fred commented 1 year ago

I see. Seems more possible then; I would want the API to be pretty EnC specific, and not something someone can stumble into by accident, but if we don't care about the impact on the rest of the compilation, it seems more reasonable to me.

tmat commented 1 year ago

Definitely. I don't think we actually need new API - we know that we are analyzing compilation for EnC delta when this code is called from EmitDifference.

333fred commented 10 months ago

C# been addressed. VB still need to be changed for similar fixes.