-
Notifications
You must be signed in to change notification settings - Fork 775
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
[AOT] Fix RuntimeContext warnings #4460
Changes from all commits
87cfa2a
bf8a530
b133982
236a5f1
d4c38af
bd2d709
db04bd1
260327b
6be9d0c
5dae942
ebee07a
299fc2a
e93903f
cc00b30
99c8baa
a773917
fc32b25
6ef81cd
23b77b5
6faf599
e6b016c
44bb210
8d76270
05bb4b6
8586200
452f6a6
649e39d
f0c7f44
f8714f9
7956ab1
d2024a8
b9c0b33
4120ffb
633da1b
7c1fde4
48d0b7f
19fce6a
35173c8
21495bc
586511d
9ba6626
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,66 @@ | ||
// <copyright file="RuntimeContextSlotFactory.cs" company="OpenTelemetry Authors"> | ||
// Copyright The OpenTelemetry Authors | ||
// | ||
// Licensed under the Apache License, Version 2.0 (the "License"); | ||
// you may not use this file except in compliance with the License. | ||
// You may obtain a copy of the License at | ||
// | ||
// http://www.apache.org/licenses/LICENSE-2.0 | ||
// | ||
// Unless required by applicable law or agreed to in writing, software | ||
// distributed under the License is distributed on an "AS IS" BASIS, | ||
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
// See the License for the specific language governing permissions and | ||
// limitations under the License. | ||
// </copyright> | ||
|
||
#nullable enable | ||
|
||
using System.Diagnostics.CodeAnalysis; | ||
|
||
namespace OpenTelemetry.Context | ||
{ | ||
internal abstract class RuntimeContextSlotFactory | ||
{ | ||
public abstract RuntimeContextSlot<T> Create<T>(string name); | ||
|
||
public sealed class AsyncLocalRuntimeContextSlotFactory : RuntimeContextSlotFactory | ||
{ | ||
public override RuntimeContextSlot<T> Create<T>(string name) | ||
=> new AsyncLocalRuntimeContextSlot<T>(name); | ||
} | ||
|
||
#if NETFRAMEWORK | ||
public sealed class RemotingRuntimeContextSlotFactory : RuntimeContextSlotFactory | ||
{ | ||
public override RuntimeContextSlot<T> Create<T>(string name) | ||
=> new RemotingRuntimeContextSlot<T>(name); | ||
} | ||
#endif | ||
|
||
public sealed class ThreadLocalRuntimeContextSlotFactory : RuntimeContextSlotFactory | ||
{ | ||
public override RuntimeContextSlot<T> Create<T>(string name) | ||
=> new ThreadLocalRuntimeContextSlot<T>(name); | ||
} | ||
|
||
[RequiresUnreferencedCode("ReflectionRuntimeContextSlotFactory is trimmer unsafe.")] | ||
[RequiresDynamicCode("ReflectionRuntimeContextSlotFactory requires the ability to generate new code at runtime.")] | ||
public sealed class ReflectionRuntimeContextSlotFactory : RuntimeContextSlotFactory | ||
{ | ||
private readonly Type runtimeContextSlotType; | ||
|
||
public ReflectionRuntimeContextSlotFactory(Type runtimeContextSlotType) | ||
{ | ||
this.runtimeContextSlotType = runtimeContextSlotType; | ||
} | ||
|
||
public override RuntimeContextSlot<T> Create<T>(string name) | ||
{ | ||
var type = this.runtimeContextSlotType.MakeGenericType(typeof(T)); | ||
var ctor = type.GetConstructor(new Type[] { typeof(string) })!; | ||
return (RuntimeContextSlot<T>)ctor.Invoke(new object[] { name })!; | ||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,55 @@ | ||
// <copyright file="RequiresDynamicCodeAttribute.cs" company="OpenTelemetry Authors"> | ||
// Copyright The OpenTelemetry Authors | ||
// | ||
// Licensed under the Apache License, Version 2.0 (the "License"); | ||
// you may not use this file except in compliance with the License. | ||
// You may obtain a copy of the License at | ||
// | ||
// http://www.apache.org/licenses/LICENSE-2.0 | ||
// | ||
// Unless required by applicable law or agreed to in writing, software | ||
// distributed under the License is distributed on an "AS IS" BASIS, | ||
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
// See the License for the specific language governing permissions and | ||
// limitations under the License. | ||
// </copyright> | ||
|
||
#nullable enable | ||
#if !NET7_0_OR_GREATER | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't we need a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This attribute is in the runtime of .net7.0 and .net8.0: https://learn.microsoft.com/en-us/dotnet/api/system.diagnostics.codeanalysis.requiresdynamiccodeattribute?view=net-7.0 Therefore, we will only need to compile this file if it is not net7.0 or greater. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's basically future proofing the code. That way if/when we add a net7.0+ target, this code doesn't get compiled for it. Yes it isn't strictly necessary now, but it is saving some effort/confusion for a future dev who adds a new TFM. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This kind of forces us to always add the new target to the API project as well. If in the future, the SDK project also ends up using this attribute, we wouldn't be able to add a net7.0+ target only to the SDK project as it would complain about the attribute existing in both There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you have a recommendation to resolve this? The only other option I could imagine is to
Using InternalsVisibleTo across separate NuGet packages isn't recommended, since the package versions can get out of sync and users will see errors. In general, matching TFMs across these packages is preferrable. It makes maintenance much easier. It is how we manage our packages we ship out of dotnet/runtime. They all have a consistent set of targets. For the 8.0-* packages it is: |
||
namespace System.Diagnostics.CodeAnalysis | ||
{ | ||
/// <summary> | ||
/// Indicates that the specified method requires the ability to generate new code at runtime, | ||
/// for example through <see cref="Reflection"/>. | ||
/// </summary> | ||
/// <remarks> | ||
/// This allows tools to understand which methods are unsafe to call when compiling ahead of time. | ||
/// </remarks> | ||
[AttributeUsage(AttributeTargets.Method | AttributeTargets.Constructor | AttributeTargets.Class, Inherited = false)] | ||
internal sealed class RequiresDynamicCodeAttribute : Attribute | ||
{ | ||
/// <summary> | ||
/// Initializes a new instance of the <see cref="RequiresDynamicCodeAttribute"/> class | ||
/// with the specified message. | ||
/// </summary> | ||
/// <param name="message"> | ||
/// A message that contains information about the usage of dynamic code. | ||
/// </param> | ||
public RequiresDynamicCodeAttribute(string message) | ||
{ | ||
this.Message = message; | ||
} | ||
|
||
/// <summary> | ||
/// Gets a message that contains information about the usage of dynamic code. | ||
/// </summary> | ||
public string Message { get; } | ||
|
||
/// <summary> | ||
/// Gets or sets an optional URL that contains more information about the method, | ||
/// why it requires dynamic code, and what options a consumer has to deal with it. | ||
/// </summary> | ||
public string? Url { get; set; } | ||
} | ||
} | ||
#endif |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,56 @@ | ||
// <copyright file="RequiresUnreferencedCodeAttribute.cs" company="OpenTelemetry Authors"> | ||
// Copyright The OpenTelemetry Authors | ||
// | ||
// Licensed under the Apache License, Version 2.0 (the "License"); | ||
// you may not use this file except in compliance with the License. | ||
// You may obtain a copy of the License at | ||
// | ||
// http://www.apache.org/licenses/LICENSE-2.0 | ||
// | ||
// Unless required by applicable law or agreed to in writing, software | ||
// distributed under the License is distributed on an "AS IS" BASIS, | ||
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
// See the License for the specific language governing permissions and | ||
// limitations under the License. | ||
// </copyright> | ||
|
||
#nullable enable | ||
#if NETFRAMEWORK || NETSTANDARD2_0_OR_GREATER | ||
namespace System.Diagnostics.CodeAnalysis | ||
alanwest marked this conversation as resolved.
Show resolved
Hide resolved
|
||
{ | ||
/// <summary> | ||
/// Indicates that the specified method requires dynamic access to code that is not referenced | ||
/// statically, for example through <see cref="Reflection"/>. | ||
/// </summary> | ||
/// <remarks> | ||
/// This allows tools to understand which methods are unsafe to call when removing unreferenced | ||
/// code from an application. | ||
/// </remarks> | ||
[AttributeUsage(AttributeTargets.Method | AttributeTargets.Constructor | AttributeTargets.Class, Inherited = false)] | ||
internal sealed class RequiresUnreferencedCodeAttribute : Attribute | ||
{ | ||
/// <summary> | ||
/// Initializes a new instance of the <see cref="RequiresUnreferencedCodeAttribute"/> class | ||
/// with the specified message. | ||
/// </summary> | ||
/// <param name="message"> | ||
/// A message that contains information about the usage of unreferenced code. | ||
/// </param> | ||
public RequiresUnreferencedCodeAttribute(string message) | ||
{ | ||
this.Message = message; | ||
} | ||
|
||
/// <summary> | ||
/// Gets a message that contains information about the usage of unreferenced code. | ||
/// </summary> | ||
public string Message { get; } | ||
|
||
/// <summary> | ||
/// Gets or sets an optional URL that contains more information about the method, | ||
/// why it requires unreferenced code, and what options a consumer has to deal with it. | ||
/// </summary> | ||
public string? Url { get; set; } | ||
} | ||
} | ||
#endif |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,6 +17,9 @@ | |
#nullable enable | ||
|
||
using System.Diagnostics; | ||
#if NETSTANDARD2_1_OR_GREATER || NET6_0_OR_GREATER | ||
using System.Diagnostics.CodeAnalysis; | ||
#endif | ||
using System.Text; | ||
using Microsoft.Extensions.DependencyInjection; | ||
using OpenTelemetry.Internal; | ||
|
@@ -201,7 +204,12 @@ public bool ContainsBatchProcessor(BaseProcessor<LogRecord> processor) | |
} | ||
|
||
/// <inheritdoc /> | ||
protected override bool TryCreateLogger(string? name, out Logger? logger) | ||
protected override bool TryCreateLogger( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
string? name, | ||
#if NETSTANDARD2_1_OR_GREATER || NET6_0_OR_GREATER | ||
[NotNullWhen(true)] | ||
#endif | ||
out Logger? logger) | ||
{ | ||
logger = new LoggerSdk(this, name); | ||
return true; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should scope it down to individual methods instead of suppressing the warning for the entire file.
Consider using the SuppressMessage attribute for each of the methods that it warns about.