Initial stubs for an LSIF generator that runs as part of the normal compilation pass. #49046

CyrusNajmabadi · 2020-10-29T17:43:36Z

Should be reviewed one commit at a time. The basic idea, after talking to @jasonmalinowski @chsienki is to allow analyzers to add additional source files to a compilation that can be written out if the user supplies a generated-output directory to the compiler.

This way they run as a post pass during the analyzer pass (so after source generators), seeing the entire real compilation. They also only generate addtional-files, not source-files, so they have no impact on the meaning of any symbols.

This should be reviewed one commit at a time. Sending out now so i can get feedback before digging myself into too deep a hole.

src/Compilers/Core/Portable/CommandLine/CommonCompiler.cs

CyrusNajmabadi · 2020-10-29T17:51:22Z

src/Compilers/Core/Portable/CommandLine/CommonCompiler.cs

+                        lock (generatedAdditionalFiles)
+                            generatedAdditionalFiles.Add(tuple);
+                    };
+                }


this part is new.

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerDriver.cs

src/Compilers/Core/Portable/DiagnosticAnalyzer/DiagnosticAnalysisContext.cs

Co-authored-by: Jason Malinowski <jason@jason-m.com>

CyrusNajmabadi · 2020-11-10T16:40:20Z

src/Compilers/Core/Portable/DiagnosticAnalyzer/ArtifactGenerator.cs

+        public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics
+            => ImmutableArray<DiagnosticDescriptor>.Empty;
+
+        public sealed override void Initialize(AnalysisContext context)


sealed as we do not want the subclasses to control this at all. they only override the various GenerateArtifacts overloads.

src/Compilers/Core/Portable/DiagnosticAnalyzer/ArtifactGenerator.cs

…slyn into lsifGeneration

chsienki · 2021-03-09T00:47:54Z

src/Compilers/Core/Portable/PublicAPI.Unshipped.txt

+Microsoft.CodeAnalysis.Diagnostics.ArtifactContext.WriteArtifact(string! fileName, string! source) -> void
+Microsoft.CodeAnalysis.Diagnostics.ArtifactContext.WriteArtifact(string! fileName, System.Action<System.IO.Stream!>! writeStream) -> void
+Microsoft.CodeAnalysis.Diagnostics.ArtifactContext.WriteArtifact(string! fileName, System.Text.StringBuilder! builder) -> void
+Microsoft.CodeAnalysis.Diagnostics.IArtifactProducer


This doesn't exist any more right?

weird! vs didn't give me any errors about this. removed.

…slyn into lsifGeneration

jasonmalinowski · 2021-03-10T00:38:26Z

FYI there's a file called "git" at the root which, being empty, means GitHub won't let me comment on the file:

jasonmalinowski

API shape seems quite good to me other than a potential concern making ArtifactContext a struct (discussed in a comment). and also some concerns about whether file locking is sufficient/wise to prevent multiple writing to artifacts. The former I'm not convinced myself whether changing is better or if it's a problem, and the latter is an implementation detail that doesn't impact the API shape.

(I want a button on GitHub which is "approve with comments but I also really didn't look at how the implementation of this all hooks up with existing code so don't pretend I reviewed that.")

jasonmalinowski · 2021-03-10T00:40:02Z

src/Compilers/CSharp/Test/CommandLine/CommandLineTests.cs

@@ -9811,7 +9811,7 @@ private static int OccurrenceCount(string source, string word)

            if (expectedInfoCount == 0)
            {
-                Assert.DoesNotContain("info", output, StringComparison.Ordinal);
+                Assert.DoesNotContain("info", output.Split('\n').First(), StringComparison.Ordinal);


What's going on with this change here? Could this now cause tests to miss something?

yes. the problem was that you might get something like a stack-trace in a compiler message (i.e. when analyzers crash we include their stack in the message). But we don't want to search that stack for things like 'info/warn/error' because the stack trace message may contain that. Instead, we just check the first line of the compiler diagnostic message as that is the part where the compiler itself prints out if this is a warning/info/error.

note: this is a major hack in teh first place as all these tests run the compiler then grok the console output. this just makes us resilient to that problem (which i was hitting).

jasonmalinowski · 2021-03-10T00:51:53Z

src/Compilers/Core/Portable/DiagnosticAnalyzer/ArtifactContext.cs

+    /// supported.  In general that will only be when a compiler is invoked with the <c>generatedartifactsout</c>
+    /// argument.


We might have to be careful if people start trying to write code that emits diagnostics because of a failure of this method. People might think "oh, I don't want people to forget to set /generatedartifactsout, let me be helpful by emitting a diagnostic!" and now they're getting diagnostics in the IDE. I'm not sure it's worth putting docs here, but maybe in any sort of guide we write up for this.

src/Compilers/Core/Portable/DiagnosticAnalyzer/ArtifactContext.cs

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalysisContext.cs

src/Compilers/Core/Portable/CommandLine/CommonCompiler.cs

CyrusNajmabadi · 2021-03-10T20:57:47Z

Feedbak addressed.

jasonmalinowski

API looks good from my perspective; IDE and compiler folks should still review the actual changes to the diagnostics engine that react to it.

src/Compilers/Core/Portable/DiagnosticAnalyzer/ArtifactContext.cs

Co-authored-by: Jason Malinowski <jason@jason-m.com>

…slyn into lsifGeneration

CyrusNajmabadi · 2021-03-11T17:11:55Z

Design meetings questions and conclusions:

Should we have the string builder overload?

Yes. There is no safety concern here as we are not taking ownership of the string builder. Instead, we are copying the data over the lifetime of the call and writing it to the stream. This saves expensive allocations and is more convenient than forcing the user to make the stream, and copy to it themselves. This does force us to doc that copying/writing is done immediately. However, that is not a bad thing, and is actually important for this domain as the purpose is to allow large scale artifacts to be written without high memory costs.

Should we ensure that file-names provided are relative only?

Yes. All writes must be under the artifact-production dir. Absolute paths are not allowed. And using .. should be prevented. We may have existing mechanisms to help here that we can reuse.

Need to plug this into GeneratorContext with same semantics as AnalysisContext.
Add cancellation support to these apis.
Add support for ReadOnlySpan
Move to TryCreateStream so plugins can gracefully fail if we cannot make a stream (e.g. 'out of handles').
Allow fine grained customization of destination directory. Multiple output paths per producer shoudl be supported.
REmove the Action callback form of teh api.

chsienki

I've created a feature branch for this work.

I'm happy with the code as-is right now, modulo the design review questions, which we will address in future PRs

CyrusNajmabadi added 5 commits October 29, 2020 09:28

Extract method

a8f06a6

Move code.

4a05e1a

Extract method

11a43bf

Extract method

a8df511

Thread through ability to add additionl files

d62fe1e

CyrusNajmabadi requested review from jasonmalinowski, mavasani and chsienki October 29, 2020 17:43

Dotnet-GitSync-Bot added the Area-Analyzers label Oct 29, 2020

Update tests

f0f5051

CyrusNajmabadi commented Oct 29, 2020

View reviewed changes

src/Compilers/Core/Portable/CommandLine/CommonCompiler.cs Show resolved Hide resolved

CyrusNajmabadi commented Oct 29, 2020

View reviewed changes

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerDriver.cs Show resolved Hide resolved

CyrusNajmabadi added 2 commits October 29, 2020 10:59

Simplify

aca7019

Rename items

3c86638

jasonmalinowski reviewed Oct 29, 2020

View reviewed changes

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerDriver.cs Outdated Show resolved Hide resolved

jasonmalinowski reviewed Oct 29, 2020

View reviewed changes

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerDriver.cs Outdated Show resolved Hide resolved

jasonmalinowski reviewed Oct 29, 2020

View reviewed changes

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerDriver.cs Outdated Show resolved Hide resolved

mavasani reviewed Oct 29, 2020

View reviewed changes

src/Compilers/Core/Portable/DiagnosticAnalyzer/DiagnosticAnalysisContext.cs Outdated Show resolved Hide resolved

mavasani reviewed Oct 29, 2020

View reviewed changes

src/Compilers/Core/Portable/DiagnosticAnalyzer/DiagnosticAnalysisContext.cs Outdated Show resolved Hide resolved

runfoapp bot mentioned this pull request Oct 29, 2020

Tests broken by code changes cause CI to fail #49022

Closed

CyrusNajmabadi and others added 4 commits November 10, 2020 07:51

Merge remote-tracking branch 'upstream/master' into lsifGeneration

5da8541

Move to separate context types

c64e0ba

Remove loc string

1de202b

Update src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerDriver.cs

98bc3c8

Co-authored-by: Jason Malinowski <jason@jason-m.com>

CyrusNajmabadi commented Nov 10, 2020

View reviewed changes

src/Compilers/Core/Portable/DiagnosticAnalyzer/ArtifactGenerator.cs Outdated Show resolved Hide resolved

CyrusNajmabadi added 3 commits November 10, 2020 08:42

Remove

5e8ef58

Merge branch 'lsifGeneration' of https://github.com/CyrusNajmabadi/ro…

320a953

…slyn into lsifGeneration

NRT

3e20046

chsienki reviewed Mar 9, 2021

View reviewed changes

CyrusNajmabadi added 3 commits March 9, 2021 10:20

Remove entries

bb7b28d

Merge branch 'lsifGeneration' of https://github.com/CyrusNajmabadi/ro…

ab76abb

…slyn into lsifGeneration

Make sure to always close the streams

c667720

Remove

4e20c17

jasonmalinowski reviewed Mar 10, 2021

View reviewed changes

CyrusNajmabadi added 7 commits March 10, 2021 11:59

Switch to a class

bee9833

Rename

620745f

Throw if multiple opens to same file

3efbba0

Fix tests

47656fa

Simplify tests

b4738ea

Simplify tests

f36ce07

Simplify

8fa2859

CyrusNajmabadi requested review from jasonmalinowski, chsienki and 333fred March 10, 2021 20:58

jasonmalinowski approved these changes Mar 10, 2021

View reviewed changes

src/Compilers/Core/Portable/DiagnosticAnalyzer/ArtifactContext.cs Outdated Show resolved Hide resolved

CyrusNajmabadi and others added 5 commits March 10, 2021 13:46

Update src/Compilers/Core/Portable/DiagnosticAnalyzer/ArtifactContext.cs

2945027

Co-authored-by: Jason Malinowski <jason@jason-m.com>

Remove

24846a8

Merge branch 'lsifGeneration' of https://github.com/CyrusNajmabadi/ro…

c6d198d

…slyn into lsifGeneration

Docs

42e6093

Using

7ea60f3

chsienki changed the base branch from main to features/artifact-producer March 12, 2021 17:56

chsienki approved these changes Mar 12, 2021

View reviewed changes

chsienki mentioned this pull request Jan 26, 2022

Support generating non-source files #57608

Open

CyrusNajmabadi closed this Jun 3, 2022

CyrusNajmabadi deleted the lsifGeneration branch June 3, 2022 17:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial stubs for an LSIF generator that runs as part of the normal compilation pass. #49046

Initial stubs for an LSIF generator that runs as part of the normal compilation pass. #49046

CyrusNajmabadi commented Oct 29, 2020 •

edited

Loading

CyrusNajmabadi Oct 29, 2020

CyrusNajmabadi Nov 10, 2020

chsienki Mar 9, 2021

CyrusNajmabadi Mar 9, 2021

jasonmalinowski commented Mar 10, 2021

jasonmalinowski left a comment

jasonmalinowski Mar 10, 2021

CyrusNajmabadi Mar 10, 2021

jasonmalinowski Mar 10, 2021

CyrusNajmabadi commented Mar 10, 2021

jasonmalinowski left a comment

CyrusNajmabadi commented Mar 11, 2021

chsienki left a comment

		/// supported. In general that will only be when a compiler is invoked with the <c>generatedartifactsout</c>
		/// argument.

Initial stubs for an LSIF generator that runs as part of the normal compilation pass. #49046

Initial stubs for an LSIF generator that runs as part of the normal compilation pass. #49046

Conversation

CyrusNajmabadi commented Oct 29, 2020 • edited Loading

CyrusNajmabadi Oct 29, 2020

Choose a reason for hiding this comment

CyrusNajmabadi Nov 10, 2020

Choose a reason for hiding this comment

chsienki Mar 9, 2021

Choose a reason for hiding this comment

CyrusNajmabadi Mar 9, 2021

Choose a reason for hiding this comment

jasonmalinowski commented Mar 10, 2021

jasonmalinowski left a comment

Choose a reason for hiding this comment

jasonmalinowski Mar 10, 2021

Choose a reason for hiding this comment

CyrusNajmabadi Mar 10, 2021

Choose a reason for hiding this comment

jasonmalinowski Mar 10, 2021

Choose a reason for hiding this comment

CyrusNajmabadi commented Mar 10, 2021

jasonmalinowski left a comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Mar 11, 2021

chsienki left a comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Oct 29, 2020 •

edited

Loading