ConfigurationBindingGenerator should move to GetInterceptableLocation API

[RikkiGibson]

Roslyn just introduced a new API for creating [InterceptsLocation] attributes in generated code. This uses a new opaque encoding for the location.

We'd like our partners to onboard to this new API as soon as is feasible. (We just merged it so may be a day or two before arcade will start handing out compilers which contain it.)

The old path-line-column approach is going to be deprecated and support removed prior to stable release of the feature.

[tarekgh]

CC @ericstj

[ericstj]

We should adopt this in the 9.0 codebase and potentially in 8.0 servicing - in case that's impacted by the removal of the file/line/character support. I think it will be since we can target net8.0 from the latest VS.

We might need to call the new API through light-up to avoid binding to the new compiler version - if we can't garuntee it's availability everywhere we need it.

@RikkiGibson which versions of VS will get this update?

[RikkiGibson]

The GetInterceptableLocation API will ship in VS 17.11 preview 1.

[ericstj]

I think we'll need to call it through lightup for some time. Check for the presence of GetInterceptableLocation and use it if present. That way the generator doesn't take a hard dependency on the new compiler.

That's the only way we can call it for the 8.0 generator. If the old path-line-column approach is pulled out eventually then we'll need to backport this to 8.0 so that it will continue to work.

[steveharter]

@RikkiGibson I started working on this and see that for one of the tests under Net462 tests, the encoded value isn't stable.

See main...steveharter:runtime:NewInterceptors

I didn't do much debugging, but thought I would put this out there right away.

file: BindConfiguration.generated.txt

[InterceptsLocation(1, "FzkxwRstvs6328ZsAVyIXIMBAABzcmMtMC5jcw==")]
public static OptionsBuilder<TOptions> BindConfiguration<TOptions>(this OptionsBuilder<TOptions> optionsBuilder, string configSectionPath, Action<BinderOptions>? configureBinder = null) where TOptions : class

First the test will fail with:
Expected Line:
        [InterceptsLocation(1, "FzkxwRstvs6328ZsAVyIXIMBAABzcmMtMC5jcw==")]
Actual Line:
        [InterceptsLocation(1, "l8J97SL0L7K7qKFZC0gWZoMBAABzcmMtMC5jcw==")]

If I change the expected to the actual then it flips around:
Expected Line:
        [InterceptsLocation(1, "l8J97SL0L7K7qKFZC0gWZoMBAABzcmMtMC5jcw==")]
Actual Line:
        [InterceptsLocation(1, "FzkxwRstvs6328ZsAVyIXIMBAABzcmMtMC5jcw==")]

[RikkiGibson]

Is the interceptor referring to a call within the same file? If so things aren't gonna work because the interceptor uses the content checksum of the target file to identify it.

Otherwise we'll have to look for why the target file content checksum is changing between runs.

[ericstj]

@jardpar @RikkiGibson - my impression is that this is must fix for this release because

The old path-line-column approach is going to be deprecated and support removed prior to stable release of the feature.

Please confirm. If we should wait for the new API to ready we can delay this and prioritize other work.

[steveharter]

@RikkiGibson it turns out it was a test problem - the same test had different C# source input so the hash changed. For now, I commented out that test. If we want to keep that test, I'll need another baseline.

[steveharter]

The old path-line-column approach is going to be deprecated and support removed prior to stable release of the feature.

@RikkiGibson will the attribute constructor be removed when this is done or will it just not work at that time?

Also, what do you recommend for testing? In a draft PR at #104180 that uses the old path-line-column if it is present, otherwise it uses the new GetInterceptableLocation API. However, I don't have a good way to say run the old path-line-column since it may not work in the future -- i.e. is there a way to see if that will work or not?

[RikkiGibson]

When we drop support, you will still be able to declare the overload for path-line-column, but the compiler will just stop respecting it. We might issue an error to make it clear that it's not supported.

Probably want @jaredpar to answer questions re: timeline for dropping that support. We might need to leave it in thru the support period for .NET 8.

[steveharter]

Currently the linked PR has baselines for both the "version 0" and the new "version 1" approaches. It auto-detects based on whether GetInterceptableLocation is present but can be forced to use "version 0" through an existing compilation switch and a new environment variable.

This is the most flexible approach, although adds a bit of infrastructure and noise with two baselines. The flexibility means the "version 0" attribute can be deprecated at any point once the PR is in v9 + ported to v8 and shipped in a service release.

[tarekgh]

but can be forced to use "version 0" through an existing compilation switch and a new environment variable.

Isn't the compiler team planning to obsolete version 0 at some point? I am asking to know if we introduce a switch that will be obsoleted later?

[jaredpar]

Isn't the compiler team planning to obsolete version 0 at some point?

Yes. By .NET 10 RTM there will be some obsoletion warning / error about this.

[steveharter]

The commit went in for v9; we will need to backport to v8.

[steveharter]

@RikkiGibson the line endings needed to be normalized in the tests so we have a consistent baseline for our tests otherwise the Linux tests for example would fail due to the source hash being added to the [InterceptsLocation]`'s base64 string argument.

I see this was discussed in the feature request at dotnet/roslyn#72133 (comment)

This test encumbrance could be avoided I assume if the hash normalized the line endings prior. Would this also mean that a Windows vs. Linux developer would run into this issue - each would want to change the [InterceptsLocation]'s base64 string?