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

EE doesn't handle methods that span multiple documents correctly #64098

Open
tmat opened this issue Sep 16, 2022 · 2 comments
Open

EE doesn't handle methods that span multiple documents correctly #64098

tmat opened this issue Sep 16, 2022 · 2 comments

Comments

@tmat
Copy link
Member

tmat commented Sep 16, 2022

MethodDebugInfo.ReadMethodDebugInfo
Incorrectly assumes that a method body spans only a single document:
https://github.com/dotnet/roslyn/blame/main/src/ExpressionEvaluator/Core/Source/ExpressionCompiler/PDB/MethodDebugInfo.Native.cs#L173

This is not the case for methods with #line directives, e.g.:

         public void InitializeComponent() {
            if (_contentLoaded) {
                return;
            }
            _contentLoaded = true;
            System.Uri resourceLocater = new System.Uri("/WpfApp5;component/mainwindow.xaml", System.UriKind.Relative);
            
            #line 1 "..\..\MainWindow.xaml"
            System.Windows.Application.LoadComponent(this, resourceLocater);
            
            #line default
            #line hidden
        }

Issue introduced by #62812

Solution proposal

We define a new kind of ImportScope, say Containing Document scope (kind 9). The blob of this import scope would encode the row id of the Document that contains the source code associated with this scope.

Import scopes are used to capture usings/Imports available at given LocalScope. Local scopes span a range of IL within a method and point to the ImportScope applicable to that IL range.

These constructs allow the compiler to scope usings to a whole method or a specific IL range. The latter is needed for emitting correct debug information for constructors of partial types spanning multiple files that contain field/property initializers. Each initializer IL can potentially be in scope of different usings. Note that the compiler does not emit this information correctly in this scenario, see #2846.

The EE finds the ImportScope chain associated with the current IL instruction to determine what usings are in scope for that instruction. If the chain contains Containing Document scope, it would use the inner-most one to determine what file types are available in the scope. If no Containing Document scope is present it would use the Document associated with the IL instruction (via the covering sequence point) for resolving file types.

The Containing Document scope is thus optional and the compiler should omit it if it is not needed to reduce the size of the PDB -- i.e. there are no #line directives or partial type field/property initializers present in the source file.

Relates to test plan #60819 (file-local types)

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Interactive untriaged Issues and PRs which have not yet been triaged by a lead labels Sep 16, 2022
@RikkiGibson
Copy link
Contributor

We will need to figure out how to get the document which contains the C# source text for the method implementation. It was suggested that doing this in general could require reverse mapping all the line directives in the project.

@RikkiGibson
Copy link
Contributor

RikkiGibson commented Sep 16, 2022

It might be best to add a new CDI to do this properly. When a method is in the same file as a file type, we would add an entry somewhere which indicates the original source file for the method, which we can use to look up the file type.

A new kind of ImportScope representing the declaring document might work.

@arunchndr arunchndr added Bug and removed untriaged Issues and PRs which have not yet been triaged by a lead labels Sep 19, 2022
@jcouv jcouv added the New Language Feature - File-Local Types File-local types (file types) label Oct 7, 2022
@RikkiGibson RikkiGibson added this to the Compiler.Next milestone Nov 15, 2022
@jaredpar jaredpar modified the milestones: Compiler.Next, Backlog Sep 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants