Emit an additional `ExcludeFromCodeCoverageAttribute` for generated methods

[vzarytovskii]

By default, we only emit CompilerGeneratedAttribute for generated methods (CompareTo, Equals, etc.), and since we also generate sequence points for them, VS code analysis shows them in the report:

The only user-added method in picture above is Create(int).

We should be emitting an ExcludeFromCodeCoverageAttribute for generated methods, so they can be excluded from the report.

[dsyme]

@jakubch1 says

We exclude all methods with no sequence points on it. Any reason why you include sequence points on those generated methods?

Hmmm good point. It’s just historical I think, attempt to get some kind of debugging for these, when perhaps it has the opposite effect to what we want.

@vzarytovskii Let's experiment with removing the sequence points for these methods. I can point you to the relevant code or spike it with you

[vzarytovskii]

@vzarytovskii Let's experiment with removing the sequence points for these methods. I can point you to the relevant code or spike it with you

You can just point me to it, I can tinker with it.

[dsyme]

@jakubch1 What about FeeFee debug points - do you omit methods that only contain those?

You can just point me to it, I can tinker with it.

@vzarytovskii Here are the places I remember

1. We currently always strive to emit a debug sequence point for all code including the stuff that happens in AugmentWithHashCompare.fs or anything coming out of optimization etc. For example I believe the SPAlways at https://github.com/dotnet/fsharp/blob/main/src/fsharp/IlxGen.fs#L4801 has the effect that a debug point based on the range of the method will always appear.

2. We always make space for a debug point at the head of each method by emitting a FeeFee debug sequence point. This may later be replaced by a "real" debug point here. For more information on FeeFee debug points see this old blog post. It is likely that we need to change this as I presume the presence of even a FeeFee debug point will count as "having sequence points" per my question above.

3. The codegen for discriminated unions NewXYZ, IsXYZ, Tag etc. members also lays down debug points, see https://github.com/dotnet/fsharp/blob/main/src/fsharp/ilx/EraseUnions.fs#L644 for the data that drives this and track back and forth to the production and consumption points of that data.

4. There can be come codegen involved in closures, e.g. long curried closures fun a b c d e f -> ... can generate an intermediate closure class. Track back and forth from here. It looks like these are getting debug points here and here.

There are other things we should check too, e.g. the various items of code for state machine lowering.

Looking at this, I actually think we shuold approach this systematically and holistically - we should make a table in the compiler guide of every single item of non-obvious code generation we do, and list what we expect the debug experience, profiling experience, emitted attributes and debug points to be. Check everything, document everything.

[jakubch1]

We do have some logic based on:

private const int JustMyCodeWithNoSource = 0xf00f00;

and then:

SourceFile = RawSequencePoints.LastOrDefault(s => !s.IsHidden && !string.IsNullOrEmpty(s.SourceFilePath) && s.StartLine != SequencePoint.HiddenLine && s.StartLine != JustMyCodeWithNoSource)?.SourceFilePath;

                _hasSymbolicInformation = (int)Id != -1 && Id != 0
                    && MetadataToken != 0
                    && !string.IsNullOrEmpty(Name)
                    && !string.IsNullOrEmpty(SourceFile);

If I understand it correctly if all sequence points are like this we exclude method.

[dsyme]

@jakubch1 Thanks! Note also SequencePoint.HiddenLine in that logic is 0xFeeFee (link)

I'd like to understand the difference between JustMyCodeWithNoSource and SequencePoint.HiddenLine - when does the C# compiler emit each of these? Certainly for much of our codegen JustMyCodeWithNoSource would seem to make sense.

Also I wonder when s.IsHidden is true.

[jakubch1]

public bool IsHidden => StartLine == 16707566; (System.Reflection.Metadata)

[jakubch1]

It looks like we can simplify our logic. We are checking it twice.

[dsyme]

It looks like we can simplify our logic. We are checking it twice.

Yup, thanks

[KathleenDollard]

I wanted to ping this issue to see where we stand

[dsyme]

In #12432 I've been doing a lot of relevant work, though it's not finalised yet. So I think a fix will come either as that, or immediately after that.