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

InterceptableLocation.GetDisplayLocation breaks deterministic build #76126

Open ThomsonTan opened 6 days ago

ThomsonTan commented 6 days ago

Version Used: .NET SDK 9.0.100

The current implementation of InterceptableLocation.GetDisplayName() which is used in source generator to output human-readable location for the generated code, but it outputs full path which breaks deterministic build as the the generated source code could be embedded to the PDB file. Should the output location be normalized to restore the deterministic build?

https://github.com/dotnet/roslyn/blob/6cc106c0eaa9b0ae070dba3138a23aeab9b50c13/src/Compilers/CSharp/Portable/Utilities/InterceptableLocation.cs#L74-L78

https://github.com/dotnet/roslyn/blob/6cc106c0eaa9b0ae070dba3138a23aeab9b50c13/src/Compilers/CSharp/Portable/Utilities/InterceptableLocation.cs#L35-L37

The generated non-deterministic source code looks like below.

    [GeneratedCode("Microsoft.Extensions.Configuration.Binder.SourceGeneration", "9.0.11.2809")]
    file static class BindingExtensions
    {
        #region IConfiguration extensions.
        /// <summary>Attempts to bind the given object instance to configuration values by matching property names against configuration keys recursively.</summary>
        [InterceptsLocation(1, "aSfEOMF93KOjHfGLzFdenPgFAABPbmVDb2xsZWN0b3JMb2dFeHBvcnRQcm9jZXNzb3JCdWlsZGVyLmNz")] // D:\a\opentelemetry-dotnet-contrib\opentelemetry-dotnet-contrib\src\OpenTelemetry.Exporter.OneCollector\Logs\OneCollectorLogExportProcessorBuilder.cs(47,74)
jaredpar commented 2 days ago

but it outputs full path which breaks deterministic build

A deterministic build is when given the same inputs a build produces the same outputs. The path of source files are part of the input to a build and it's reasonable, and expected, that changing them will result in different build outputs. In the case the user wants to normalize paths the -pathmap feature is available.

That being said @RikkiGibson, in looking through this code I don't see that PathMap is being used here. Why is that? Seems odd.

RikkiGibson commented 2 days ago

It looks like applying pathmap was not specifically discussed during design (#72133) or API review feedback (https://github.com/dotnet/roslyn/issues/72133#issuecomment-2026064540). Nor in review of the implementation (#72814).

I don't recall any reason we would want to avoid pathmap, and it seems like using it is the norm when compiler is outputting a file path.

It should be straightforward to make a bugfix level change which maps the path, same as if the path were appearing in a PDB.