Fix local function called from method and state machine #2722
Conversation
The existing algorithm to recursively discover calls to local functions from a user method was incorrect for the case where a local function was called from a state machine local function's MoveNext method. These methods were being treated as roots for the discovery, as if they were user code, which caused a failure downstream when we assumed that each local function belongs to a unique user method - since a local function might be called from a user method _and_ a nested state machine local function. The existing behavior also had the issue that a local function state machine's attributes could suppress warnings from nested local functions, going against the behavior for normal nested functions in the linker.
This fixes the issues shown in the tests by letting the state machine types participate in the call graph discovery instead of being considered as roots. Now any local functions called from state machine local functions will be associated with the user method, not with the state machine MoveNext method.
| [ExpectedWarning ("IL2026", "--MethodWithRequires--", ProducedBy = ProducedBy.Trimmer)] | ||
| void LocalFunction () => MethodWithRequires (); |
There was a problem hiding this comment.
Should we create an issue for this? More as a documentation.
In theory this should be fixable, I would expect the local function to be generated on the state machine type, right?
There was a problem hiding this comment.
No, the local function actually gets generated outside the state machine type so I think it's similar to the case of non-state-machine nested functions (this one #2689 (comment)). And this seems to be the case even if it captures state from the state machine - all of the captured state is hoisted into a separate type next to the state machine. Example: https://sharplab.io/#v2:CYLg1APgAgTAjFADAAinALAbgLACg0Bsq6yAsgBQCUyA3nsg8vY2gMwA8AlgHYAuAfGSq1mjRqLHIevKcgC8yRDlySWJADIAxZMLmDOYMMtUM0cVAHZFxxgF88toA===
There was a problem hiding this comment.
I filed #2727, please feel free to add to it if I am missing anything specific to state machines.
Co-authored-by: Vitek Karas <10670590+vitek-karas@users.noreply.github.com>
|
Yeah, I don't think the C# compiler should ever reference a local function or lambda from two methods, so if it did, that would be surprising. Of course, the one exception would be two local functions referencing another local function in the same method. At the IL level, that first local function would appear to be referenced by two other methods, but those methods are themselves local functions. |
Yup, that case was handled in the original change, just not when one of those local functions was a state machine - the implementation didn't identify MoveNext methods as part of the state machine local function. |
…r#2722) * Add tests for nested functions called from nested state machines The existing algorithm to recursively discover calls to local functions from a user method was incorrect for the case where a local function was called from a state machine local function's MoveNext method. These methods were being treated as roots for the discovery, as if they were user code, which caused a failure downstream when we assumed that each local function belongs to a unique user method - since a local function might be called from a user method _and_ a nested state machine local function. The existing behavior also had the issue that a local function state machine's attributes could suppress warnings from nested local functions, going against the behavior for normal nested functions in the linker. * Don't fail on multiple user methods for same nested function * Correctly handle iterator local functions calling nested functions This fixes the issues shown in the tests by letting the state machine types participate in the call graph discovery instead of being considered as roots. Now any local functions called from state machine local functions will be associated with the user method, not with the state machine MoveNext method. * Fix test for analyzer * Update src/linker/Linker/CompilerGeneratedState.cs Co-authored-by: Vitek Karas <10670590+vitek-karas@users.noreply.github.com> * Adjust comments and docs * Fix docs Co-authored-by: Vitek Karas <10670590+vitek-karas@users.noreply.github.com> Commit migrated from dotnet/linker@c6434f6
3 participants
Fixes #2723
I wasn't able to reproduce the issue by building dotnet/runtime for wasm locally (not sure what I'm doing wrong), but the stacktrace included enough info to see the problem. I was able to observe the same failure mode with these new tests. The issue was that a local function might be called from both the user method, and a state machine local function - and this case wasn't correctly handled in the call graph discovery. See commit comments for more details.
I also made it a warning (instead of throwing an exception) if we for some reason discover multiple user methods for a local function even with the fix - just in case. This is similar to what we're doing for state machines.