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

Requires on NativeAOT has problems with CompilerGeneratedCode #68786

Closed
tlakollo opened this issue May 2, 2022 · 4 comments
Closed

Requires on NativeAOT has problems with CompilerGeneratedCode #68786

tlakollo opened this issue May 2, 2022 · 4 comments
Assignees
Labels
area-NativeAOT-coreclr needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration
Milestone

Comments

@tlakollo
Copy link
Contributor

tlakollo commented May 2, 2022

Description

Doing some testing with the Requires attribute on CompilerGeneratedCode I realized that NativeAOT has differences with linker, it will not produce warnings for code using ldftn for example:

//[ExpectedWarning("IL2026", "--MethodWithRequires--", CompilerGeneratedCode = true)]
//[ExpectedWarning("IL3050", "--MethodWithRequires--")]
static IEnumerable<int> TestLdftn()
{
    yield return 0;
    yield return 1;
    var action = new Action(MethodWithRequires);
}
[RequiresUnreferencedCode("--MethodWithRequires--")]
[RequiresDynamicCode("--MethodWithRequires--")]
static void MethodWithRequires()
{
}

It will have problems with Lazy Delegates too

// Cannot annotate fields either with RUC nor RAF therefore the warning persists
//[ExpectedWarning("IL2026", "Message from --MethodWithRequiresAndReturns--", CompilerGeneratedCode = true)]
//[ExpectedWarning("IL3050", "Message from --MethodWithRequiresAndReturns--")]
public static Lazy<string> _default = new Lazy<string>(MethodWithRequiresAndReturns);

static IEnumerable<int> TestLazyDelegate()
{
    yield return 0;
    yield return 1;
    _ = _default.Value;
}
[RequiresUnreferencedCode("Message from --MethodWithRequiresAndReturns--")]
[RequiresDynamicCode("Message from --MethodWithRequiresAndReturns--")]
public static string MethodWithRequiresAndReturns()
{
    return "string";
}

Suppression inside CompilerGeneratedCode is also not working

Reproduction Steps

Expected behavior

The warnings generated by dotnet/linker and nativeAOT should be the same

Actual behavior

NativeAOT sometimes misses generating some of the warnings and some other times it generates extra warnings

Regression?

No response

Known Workarounds

No response

Configuration

No response

Other information

No response

@tlakollo tlakollo added needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration area-NativeAOT-coreclr labels May 2, 2022
@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label May 2, 2022
@MichalStrehovsky MichalStrehovsky added this to the 7.0.0 milestone May 31, 2022
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label May 31, 2022
@MichalStrehovsky MichalStrehovsky modified the milestones: 7.0.0, 8.0.0 Aug 12, 2022
@MichalStrehovsky MichalStrehovsky moved this to High Priority in AppModel Aug 12, 2022
@sbomer sbomer self-assigned this Oct 24, 2022
@sbomer
Copy link
Member

sbomer commented Oct 25, 2022

Another problem is that nativeaot doesn't always treat RUC on method as applying to local functions within:

			[RequiresUnreferencedCode ("")]
			static void TestLocalFunctionInMethodWithRequires ()
			{
				LocalFunction ();

				void LocalFunction () => MethodWithRequires ();
			}

			static void TestAll ()
			{
				typeof (LocalFunctionsReferencedViaReflection).RequiresAll ();
			}

The TestAll method should produce two warnings for RUC - once for TestLocalFunctionInMethodWithRequires and once for MethodWithRequires , but it doesn't produce the latter.

@sbomer
Copy link
Member

sbomer commented Oct 25, 2022

NativeAot is also missing warnings for reflection access to compiler-generated code (IL2118).

@sbomer
Copy link
Member

sbomer commented Oct 25, 2022

The problems with ldftn and lazy delegates are being addressed in #77457.

vitek-karas added a commit that referenced this issue Apr 24, 2023
…s warnings between ILLink and NativeAOT (#84770)

This enables the last of the disabled `RequiresCapability` tests for NativeAOT.

The implementation basically copies the logic from ILLink on how to produce warnings when method or field is marked due to reflection access.

We decided that producing warnings due to reflection access to compiler generated methods is more trouble than benefit and thus we won't do that anymore. See reasoning in #85042.

Otherwise the behavior in NativeAOT fully copies the one in ILLink (small exceptions due to better method overload resolution in NativeAOT).

High-level behavior changes in NativeAOT:
* Finally allow the NativeAOT compiler to produce IL2110, IL2111, IL2113 and IL2115
* Avoid generating warnings for override methods if the warning is already generated on the base method (for DAM marking)
* Implement correct marking (and warnings) for "DAM on type" when the derived class has broader annotation than the base class/interface (this is mostly to avoid noise warnings)
* Rename a set of methods and classes in data flow scanner to "Token", because they're only used when a given entity is accessed through its token (so `ldtoken` and similar). This allows for more precise handling of the cases above.

Testing changes:
* Allow specifying tool exceptions for `SkipKepItemsValidationAttribute` - this allows to enable one of the reflection tests for its diagnostics coverage, avoiding the necessity to fix everything around Kept behavior yet.
* Couple of fixes in the test validator to correctly compare expected/actual diagnostics

This fixes final bits of #68786
Fixes #68688
This helps with #82447
@vitek-karas
Copy link
Member

The remaining bits are fixed in #84770

@ghost ghost locked as resolved and limited conversation to collaborators May 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-NativeAOT-coreclr needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration
Projects
Archived in project
Development

No branches or pull requests

4 participants