Add VerifyMethodBody helper as a replacement of VerifyIL #66536
Conversation
VerifyMethodBody verifies both IL and sequence points without requiring to specify sequencePoints and source arguments like VerifyIL. Instead of looking up method in the PDB by its metadata name it uses method token. The tokens are made available via TestData by exposing MetadataWriter used for emit. The source is retrieved from the compilation using the document name stored in the PDB.
| return result; | ||
| } | ||
|
|
||
| public static Dictionary<int, string> GetSequencePointMarkers(XElement methodXml, Func<string, SourceText> getSource) |
There was a problem hiding this comment.
Feels like these two GetSequencePointMarkers methods are 90% identical. Can we factor them into a single implementation?
There was a problem hiding this comment.
They are actually quite different. I don't see benefit of a single implementation, especially since the GetSequencePointMarkers is only there for existing tests and should ideally be removed at some point.
There was a problem hiding this comment.
They are actually quite different
I'm not convinced. I did a diff between the two and the only two differences are the ordering of two if blocks and which strings we decide to print out (either fixed strings like "~" or strings with some source).
I'll let second reviewer chime in.
There was a problem hiding this comment.
Is there a reason why the two foreach blocks are swapped?
the
GetSequencePointMarkersis only there for existing tests and should ideally be removed at some point
I'm not sure we will ever refactor all the tests to get rid of this method, so it makes sense to me to share the code if possible.
There was a problem hiding this comment.
Is there a reason why the two foreach blocks are swapped?
I don't know, the logic was implemented in two separate code paths before my change. I just moved it to a separate method. I really don't think there is any value in trying to unify them.
There was a problem hiding this comment.
I'm not sure we will ever refactor all the tests to get rid of this method, so it makes sense to me to share the code if possible.
There is not that many of these tests.
|
@dotnet/roslyn-compiler for second review. Thanks |
2 similar comments
|
@dotnet/roslyn-compiler for second review. Thanks |
|
@dotnet/roslyn-compiler for second review. Thanks |
| @@ -22416,9 +22416,9 @@ struct B | |||
| // (10,9): error CS8374: Cannot ref-assign 'this' to 'F' because 'this' has a narrower escape scope than 'F'. | |||
| // r.F = ref this; // 1 | |||
| Diagnostic(ErrorCode.ERR_RefAssignNarrower, "r.F = ref this").WithArguments("F", "this").WithLocation(10, 9), | |||
| // (15,6): warning CS0436: The type 'UnscopedRefAttribute' in '' conflicts with the imported type 'UnscopedRefAttribute' in 'System.Runtime, Version=7.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a'. Using the type defined in ''. | |||
| // (15,6): warning CS0436: The type 'UnscopedRefAttribute' in '1.cs' conflicts with the imported type 'UnscopedRefAttribute' in 'System.Runtime, Version=7.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a'. Using the type defined in ''. | |||
There was a problem hiding this comment.
I assume the error message has the file name in both places? (This applies also to SymbolErrorTests, although it's probably not necessary to fix the comments.)
| // (15,6): warning CS0436: The type 'UnscopedRefAttribute' in '1.cs' conflicts with the imported type 'UnscopedRefAttribute' in 'System.Runtime, Version=7.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a'. Using the type defined in ''. | |
| // (15,6): warning CS0436: The type 'UnscopedRefAttribute' in '1.cs' conflicts with the imported type 'UnscopedRefAttribute' in 'System.Runtime, Version=7.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a'. Using the type defined in '1.cs'. |
ghost
added this to the 
* upstream/main: (547 commits) Add VerifyMethodBody helper as a replacement of VerifyIL (dotnet#66536) don't offer rename for ctor snippet (dotnet#66704) Get `ConditionalAttribute` type only once per compilation Convert language-specific option types to records (dotnet#66633) Enable MSBuild COMM log (dotnet#66708) ⚡️Share AssemblyloadTestFixture on Framework (dotnet#66684) NRT Don't overwrite binary logs in CI (dotnet#66683) Fix 'Generate Enum Member' to work in a 'Color Color' case. Find bitwise operators accessed through logical operators in FAR Semantic snippets - `propg` and `propi` snippets (dotnet#65979) NRT Properly classify aliases in quick info Fix Add tests Add support for finding collection initialiers with FAR Do not suggest replacing lambda with method group if the invoked mehod has `Conditional` attribute Revert "Record added node output states as new (dotnet#61575)" (dotnet#66696) Record added node output states as new (dotnet#61575) Fix formatting issue with convert-to-full-prop ...
VerifyMethodBody verifies both IL and sequence points without requiring to specify sequencePoints and source arguments like VerifyIL. Instead of looking up method in the PDB by its metadata name it uses method token. The tokens are made available via TestData by exposing MetadataWriter used for emit. The source is retrieved from the compilation using the document name stored in the PDB.