Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Finish trim annotation #2027

Merged
merged 3 commits into from
May 7, 2024
Merged

Conversation

agocke
Copy link
Contributor

@agocke agocke commented Mar 18, 2024

There's one trim warning that the analyzer didn't catch (bug) that the
AOT compiler does. Unfortunately, the warning isn't easily resolvable --
it implies that you can no longer use deconstructed compiler-generated
types (like anonymous types) through the various logging calls.

There's unfortunately no way to perfectly resolve this. This PR proposes
a behavioral change for trimming -- when trimming, compiler-generated
types will be logged like scalars, instead of being deconstructed. This
is not ideal. Libraries should not have significantly different behavior
when trimming without a warning or some other diagnostic being produced.

Unfortunately, the contract implied by the existing code is much too
complicated to be encoded in the trimmer. The contract is that types do
not need to preserve reflection metadata, except for compiler-generated
types, which need public properties. However, the definition of
compiler-generated types is not well-defined. Encoding a heuristic in
Serilog is one thing -- encoding the heuristic in the .NET runtime is
another.

Given the constraints, this seems like the best option.

There's one trim warning that the analyzer didn't catch (bug) that the
AOT compiler does. Unfortunately, the warning isn't easily resolvable --
it implies that you can no longer use deconstructed compiler-generated
types (like anonymous types) through the various logging calls.

There's unfortunately no way to perfectly resolve this. This PR proposes
a behavioral change for trimming -- when trimming, compiler-generated
types will be logged like scalars, instead of being deconstructed. This
is not ideal. Libraries should not have significantly different behavior
when trimming without a warning or some other diagnostic being produced.

Unfortunately, the contract implied by the existing code is much too
complicated to be encoded in the trimmer. The contract is that types do
not need to preserve reflection metadata, except for compiler-generated
types, which need public properties. However, the definition of
compiler-generated types is not well-defined. Encoding a heuristic in
Serilog is one thing -- encoding the heuristic in the .NET runtime is
another.

Given the constraints, this seems like the best option.
@nblumhardt
Copy link
Member

Thanks for forging ahead with this, @agocke; sorry it's taken so long to get eyes on it! Appreciate the effort - it's a rather nasty area to dig into, having you look into this helps enormously.

dev is now "v4" so it seems like a good point in time to introduce the switch 👍

@nblumhardt nblumhardt merged commit 1660691 into serilog:dev May 7, 2024
1 check passed
@agocke agocke deleted the finish-trim-annotation branch May 9, 2024 03:40
@agocke
Copy link
Contributor Author

agocke commented May 9, 2024

I should note one more option -- I didn't realize how popular anonymous types are in Serilog, and this implementation kind-of silently breaks that scenario. An alternative would be to throw a NotSupportedException or similar in the case that you try to serialize an anonymous type. That would make it clear to the user that this case isn't supported.

@nblumhardt nblumhardt mentioned this pull request May 27, 2024
@nblumhardt
Copy link
Member

See also #2068, switch name has changed slightly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants