Do not allow DynamicCodeSupport=true for NativeAOT

[MichalStrehovsky]

Cc @dotnet/ilc-contrib

[dotnet-policy-service]

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

[MichalStrehovsky]

This has more diffs than needed because I also fixed the formatting.

Do we have a plan for what to do with the illinker formatting? I don't mind illinker's charming odd formatting but I do care when the formatting leaks out. This is not the first or second or third time it leaked out.

We also leak it into places like learn.microsoft.com and can't even be consistent about it there. It makes things look sloppy.

[am11]

IMHO, we should just align the formatting to one standard repo-wide and fail at least one leg in CI matrix in PRs when this produces diffs:

runtime/eng/formatting/format.sh

Line 20 in 54ff0d2

echo "$MANAGED_FILES" | cat | xargs | sed -e 's/ /,/g' | dotnet format whitespace --include - --folder

There are no points in adhering to "historical reasons" at this point. 🙂

[MichalStrehovsky]

We very rarely squabble over formatting in this repo. I'm confident 90% of all formatting nits I ever had in PRs were with illinker devs doing non-illinker work.

There are times when people violate the formatting rules because it's done to highlight structure that would be lost if formatted according to the book. I'm pretty sure a formatting police would demotivate people from doing that because nobody wants to research how to pragma suppress the formatter (I would certainly quit doing that because I hate pragma suppressions). The formatter would likely destroy more readability than what it would bring; I don't see formatting as a general problem in the PRs I review (or in reviews I get on my PRs).

[filipnavara]

fail at least one leg in CI matrix in PRs when this produces diffs:

There are already JIT formatting jobs with clang-format when you touch any files in src/coreclr/jit. Presumably the same thing can be done here if there's an agreement.

[sbomer]

We plan to adopt runtime's formatting conventions for illink (tracking issue: #78050). The surest way to prevent current illink style from slipping in would be to add a formatting job. Much of it is muscle memory for me, and I'm likely to make mistakes even while trying to follow the runtime conventions. I'm also always happy to take style nit feedback on PRs.

[jkotas]

There are already JIT formatting jobs with clang-format when you touch any files in src/coreclr/jit. Presumably the same thing can be done here if there's an agreement.

You can enforce formatting via Roslyn analyzer. It has been enabled for libraries for quite some time. No need to have an extra formatting job.

[MichalStrehovsky]

fail at least one leg in CI matrix in PRs when this produces diffs:

There are already JIT formatting jobs with clang-format when you touch any files in src/coreclr/jit. Presumably the same thing can be done here if there's an agreement.

Here's how they work (just the PRs I was pinged on in the past half a day or so):

They:

1. Annoy people working in the codebase
2. Produce an extra dose of CO2 (notice the ❌)
3. Could be a fix to this manufactured problem. Using different coding styles in different part of the product is an entirely manufactured problem; I don't remember when I had to comment on formatting (or fix it) outside interactions with anyone exposed to illinker codebase; I think the only person who was able to maintain the separation was Marek because I don't recall issues with that but that would be an exception to a rule.

[am11]

I don't remember when I had to comment on formatting (or fix it) outside interactions with anyone exposed to illinker codebase

Could be because libs subset already enforces the formatting rules and fails the build (seen a single trailing space failing builds a few times).

Annoy people working in the codebase

Bit annoying it is, if we are in a rush. Which begs the question; why the rush merging PR which affects everyone? 🙂
I guess it is ok; better than annoying multiple people with unformatted code to save one developer's 5-10 seconds of time in active PR, who wasn't using VS or forgot to press Ctrl+K,D before Ctrl+S. They can use dotnet format <changed files> or the helper script in repo linked above (which only formats files in changeset, according to the .editorconfig and CodeAnalysis rules file; same as VS).

[MichalStrehovsky]

Could be because libs subset already enforces the formatting rules and fails the build (seen a single trailing space failing builds a few times).

99% of code I write or review doesn't have build-enforced formatting. I rarely review/work in the libs or JIT subsets. It's not that. It's just not a problem in general.