dotnet / linker

389 stars 127 forks source link

Data flow asserts if the only usage of lambda is removed due to unreachable block removal #2845

Open vitek-karas opened 2 years ago

vitek-karas commented 2 years ago
        static bool AlwaysFalse => false;

        static Func<int, int> RemovedLambdaUsage (int param = 0)
        {
            // Trigger data flow in this method
            typeof (TestType).GetProperties ();

            if (param == 0) {
                return (a) => a + 1;
            }

            if (AlwaysFalse) {
                return (a) => a + 2;  // This branch will be removed by unreachable block removal
            } else {
                return (a) => a + 3;
            }
        }

        class TestType { }

Compiler generated state caches results per type. So the first time we hit the type we will go over all of its methods, scan their IL and build lookups - one of which is the map from user method to all of the compiler generated methods it uses.

If this happens before we call UnreachableBlockOptimizer.ProcessMethod, the method body seen by the compiler generated state code will be different from the one we will eventually process in data flow (MethodBodyScanner). If the optimization removed a branch which is the only use case of a lambda (for example), then the number of compiler generated methods seen by the cache and the number seen by data flow will differ. This results in assert: https://github.com/dotnet/linker/blob/1e9494e29e2b2059cfaf85064d314536a165eb0e/src/linker/Linker.Dataflow/MethodBodyScanner.cs#L261

Currently the UnreachableBlockOptimizer is called from MarkStep.ProcessMethod - which can (and typically will) happen too late.

Please note that UnreachableBlockOptimizer doesn't remember which methods it processed! So calling it twice on the same method could be problematic - the MarkStep.ProcessMethod solves this by guarding it with the CheckProcessed flag. This means that simply calling the optimizer from compiler generated state alone is not the right solution - we either need to add a cache or do something different.

vitek-karas commented 2 years ago

https://github.com/dotnet/linker/pull/2846 added a test for this case.