Always emit conditional attributes in FCS

[auduchinok]

Fixes #3890. We analyse conditionally erased attributes like CanBeNull or NotNull from JetBrains.Annotations package and need to see all attributes before erasure. Any objections to always doing it in the compiler service?

[KevinRansom]

@auduchinok

#3890 indicated that there would be an option to enable or disable this. I'm not sure Don's comment covered the #if COMPILER_PUBLIC_API option.

Is it possible that tools developed with the FCS would want it both ways.
I.e, without the erased attribute and other FCS based tools would want it with the erased attribute?

Or is it possible, that there are no FCS scenarios that want the attributed erased?

Thanks

Kevin

[auduchinok]

@KevinRansom If I got it correctly Don didn't mind always emitting it in the service, which was another proposed way.

Another option is to make it always emit such attributes by adding COMPILER_PUBLIC_API condition directive to the code below so all attributes would be available for analysis by clients.

@auduchinok I'm fine with you adding this. A hidden command-line flag may be easiest to reduce plumbing, though I'm don't mind too much either way

I couldn't think of scenarios where FCS clients would want attributes erased. If there're such scenarios I'll change it to using an option.

[dsyme]

I couldn't think of scenarios where FCS clients would want attributes erased. If there're such scenarios I'll change it to using an option.

I now think we should use a flag. FCS can be used for compilation and may in the future be used as the basis for implementing fsc.exe and fsi.exe

[KevinRansom]

Can we add a switch pls.

[auduchinok]

@dsyme @KevinRansom Sure, done.

[KevinRansom]

looks good.

Thank you

[TIHan]

I kinda of don't like adding a specific switch for this. Why not have a switch to indicate we are running in tooling mode or compilation (fsc) mode?

[TIHan]

I'm not a fan of list option types as I usually get confused by what it could mean. The comment does help though, so it's not a mystery. Though, I prefer another bool field honestly. What you think?

[KevinRansom]

I think I'm okay with this.

[TIHan]

Why are you ok with it? (Just curious because I'm on the fence).

The more I think about it, the more I really want a new bool field i.e isConditionalErasureEnabled : bool. Nothing can be clearer than that especially when it's used in code.

Using an option gives the field conditionalDefines more responsibility.

[auduchinok]

@TIHan On the other hand using option makes one aware of situation when there may be no conditional erasure unlike a separate field that one may be unaware of.