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

Avoid producing Requires* warnings when accessing constant fields #84622

Merged
merged 1 commit into from
Apr 11, 2023

Conversation

vitek-karas
Copy link
Member

Fields can only be annotated with Requires* attributes through their type (so RUC on type and so on). But that annotation is actually not guarding access to the field itself, but it's actually guarding access to static members, including static constructor.

Constant fields don't trigger static constructor. In fact at runtime constant fields are never accessed by code. The compiler is required to inline their values instead.

So IL based scanners will never produce the above warning, only analyzer would.

This change modifies the analyzer to stop producing warings for accessing constant fields.

Fixes #84433

Fields can only be annotated with `Requires*` attributes through their type (so RUC on type and so on). But that annotation is actually not guarding access to the field itself, but it's actually guarding access to static members, including static constructor.

Constant fields don't trigger static constructor. In fact at runtime constant fields are never accessed by code. The compiler is required to inline their values instead.

So IL based scanners will never produce the above warning, only analyzer would.

This change modifies the analyzer to stop producing warings for accessing constant fields.

Fixes dotnet#84433
@vitek-karas vitek-karas added the area-Tools-ILLink .NET linker development as well as trimming analyzers label Apr 11, 2023
@vitek-karas vitek-karas added this to the 8.0.0 milestone Apr 11, 2023
@vitek-karas vitek-karas requested a review from sbomer April 11, 2023 11:37
@vitek-karas vitek-karas self-assigned this Apr 11, 2023
@ghost ghost added the linkable-framework Issues associated with delivering a linker friendly framework label Apr 11, 2023
@ghost
Copy link

ghost commented Apr 11, 2023

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

Issue Details

Fields can only be annotated with Requires* attributes through their type (so RUC on type and so on). But that annotation is actually not guarding access to the field itself, but it's actually guarding access to static members, including static constructor.

Constant fields don't trigger static constructor. In fact at runtime constant fields are never accessed by code. The compiler is required to inline their values instead.

So IL based scanners will never produce the above warning, only analyzer would.

This change modifies the analyzer to stop producing warings for accessing constant fields.

Fixes #84433

Author: vitek-karas
Assignees: vitek-karas
Labels:

area-Tools-ILLink

Milestone: 8.0.0

@ghost
Copy link

ghost commented Apr 11, 2023

Tagging subscribers to 'linkable-framework': @eerhardt, @vitek-karas, @LakshanF, @sbomer, @joperezr, @marek-safar
See info in area-owners.md if you want to be subscribed.

Issue Details

Fields can only be annotated with Requires* attributes through their type (so RUC on type and so on). But that annotation is actually not guarding access to the field itself, but it's actually guarding access to static members, including static constructor.

Constant fields don't trigger static constructor. In fact at runtime constant fields are never accessed by code. The compiler is required to inline their values instead.

So IL based scanners will never produce the above warning, only analyzer would.

This change modifies the analyzer to stop producing warings for accessing constant fields.

Fixes #84433

Author: vitek-karas
Assignees: vitek-karas
Labels:

linkable-framework, area-Tools-ILLink

Milestone: 8.0.0

@lewing
Copy link
Member

lewing commented Apr 11, 2023

@thaystg

C:\helix\work\workitem\e>set DEBUGGER_TEST_PATH=C:\helix\work\workitem\e/debugger-test 

C:\helix\work\workitem\e>set BROWSER_PATH_FOR_TESTS=C:\helix\work\correlation\chrome-win\chrome.exe 

C:\helix\work\workitem\e>set SKIP_LOG_TO_CONSOLE=1 

C:\helix\work\workitem\e>if /I [WasmDebuggerTests] == [WasmTestOnNodeJS] (call npm ci  ) 

C:\helix\work\workitem\e>dotnet.exe test DebuggerTestSuite/DebuggerTestSuite.dll "-l:trx;LogFileName=testResults.trx" "-l:console;Verbosity=normal" --results-directory "C:\helix\work\workitem\uploads\xharness-output\logs" --filter FullyQualifiedName~DebuggerTests.EvaluateOnCallFrameTests -v diag  

Welcome to .NET 8.0!
---------------------
SDK Version: 8.0.100-preview.1.23115.2

----------------
Installed an ASP.NET Core HTTPS development certificate.
To trust the certificate run 'dotnet dev-certs https --trust' (Windows and macOS only).
Learn about HTTPS: https://aka.ms/dotnet-https
----------------
Write your first app: https://aka.ms/dotnet-hello-world
Find out what's new: https://aka.ms/dotnet-whats-new
Explore documentation: https://aka.ms/dotnet-docs
Report issues and find source on GitHub: https://github.com/dotnet/core
Use 'dotnet --help' to see available commands or visit: https://aka.ms/dotnet-cli
--------------------------------------------------------------------------------------
Microsoft (R) Test Execution Command Line Tool Version 17.6.0-preview-20230126-02 (x64)
Copyright (c) Microsoft Corporation.  All rights reserved.

Starting test execution, please wait...
A total of 1 test files matched the specified pattern.

...
[EXECUTION TIMED OUT]
Exit Code:-3Executor timed out after 3000 seconds and was killed

['chrome-DebuggerTests.EvaluateOnCallFrameTests' END OF WORK ITEM LOG: Command timed out, and was killed]

@lewing
Copy link
Member

lewing commented Apr 11, 2023

@pavelsavara

[13:06:54] info: Unable to evaluate script: disconnected: unable to send message to renderer
[13:06:54] info: Unable to evaluate script: disconnected: unable to send message to renderer
[13:06:54] info: Unable to evaluate script: disconnected: unable to send message to renderer
[13:06:54] info: Unable to evaluate script: disconnected: unable to send message to renderer
[13:06:54] info: Unable to evaluate script: disconnected: unable to send message to renderer
[13:06:54] info: Unable to evaluate script: disconnected: unable to send message to renderer
[13:06:54] info: Unable to evaluate script: disconnected: unable to send message to renderer

...
[EXECUTION TIMED OUT]
Exit Code:-3Executor timed out after 1800 seconds and was killed

['WasmTestOnBrowser-System.Text.Json.Tests' END OF WORK ITEM LOG: Command timed out, and was killed]

@lewing
Copy link
Member

lewing commented Apr 11, 2023

@vitek-karas the wasm failures are unrelated

Copy link
Member

@sbomer sbomer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @vitek-karas!

@vitek-karas vitek-karas merged commit 981445c into dotnet:main Apr 11, 2023
@vitek-karas vitek-karas deleted the ConstFieldRUCAccess branch April 11, 2023 18:58
@ghost ghost locked as resolved and limited conversation to collaborators May 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Tools-ILLink .NET linker development as well as trimming analyzers linkable-framework Issues associated with delivering a linker friendly framework
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Aot Analyzer shouldn't warn when passing a const string to RequiresDynamicCode applied to Types
4 participants