C#: Generate source files from cshtml files in standalone #13957

tamasvajk · 2023-08-14T14:01:17Z

Supersedes #13722.

This PR adds cshtml/razor file handling to the standalone extractor. We're generating C# files to a temporary location from the view files with csc by running the razor source generators on all the cshtml/razor files in the repo. The generated source files are then extracted as normal source files.

csharp/extractor/Semmle.Extraction.CSharp.Standalone/BuildAnalysis.cs

csharp/extractor/Semmle.Extraction.CSharp.Standalone/Razor.cs

csharp/extractor/Semmle.Extraction.CSharp.Standalone/BuildAnalysis.cs

csharp/extractor/Semmle.Extraction.CSharp.Standalone/Razor.cs

csharp/extractor/Semmle.Extraction.CSharp.Standalone/DotNet.cs

csharp/extractor/Semmle.Extraction.CSharp.Standalone/Razor.cs

csharp/extractor/Semmle.Extraction.CSharp.Standalone/BuildAnalysis.cs

…g file

csharp/extractor/Semmle.Extraction.CSharp.DependencyFetching/DependencyManager.cs

csharp/extractor/Semmle.Extraction.CSharp.DependencyFetching/Razor.cs

+            catch
+            {
+                // Ignore
+            }


michaelnebel

This looks really really good!
It must have taken lots of reading and trial and error to get it to work!
Only some minor comments and questions.

michaelnebel · 2023-08-21T09:40:04Z

csharp/extractor/Semmle.Extraction.CSharp.DependencyFetching/DependencyManager.cs

@@ -131,6 +132,40 @@ public DependencyManager(string srcDir, IDependencyOptions options, ILogger logg
                progressMonitor.UnresolvedReference(r.Key, r.Value);
            }

+            var webViewExtractionOption = Environment.GetEnvironmentVariable("CODEQL_EXTRACTOR_CSHARP_STANDALONE_EXTRACT_WEB_VIEWS");


Should we consider to add a group of extractor options for buildless or standalone to make it easier for a user to specify this flag (not as a part of this PR)?

Maybe we should add this "option" to the standalone extractor Option and read the environment variable as a part of the creating the options object (and also make it a property on the DependencyOption) to avoid inlining the environment variable read here.

I think in the long run we'll have a couple of configuration options, so it definitely makes sense to group them. At the same time, we should try to minimize the number of options as the extractor should "just work". I don't have full confidence in this cshtml to C# generation, so I wanted to hide it behind an undocumented feature flag, but eventually I don't think there's any reason to not have this run all the time.

csharp/extractor/Semmle.Extraction.CSharp.DependencyFetching/DependencyManager.cs

csharp/extractor/Semmle.Extraction.CSharp.DependencyFetching/Sdk.cs

michaelnebel · 2023-08-21T10:25:32Z

csharp/extractor/Semmle.Extraction.CSharp.DependencyFetching/Razor.cs

+            this.progressMonitor = progressMonitor;
+            this.dotNet = dotNet;
+
+            sourceGeneratorFolder = Path.Combine(this.sdk.FullPath, "Sdks", "Microsoft.NET.Sdk.Razor", "source-generators");


Quick question: Have you found any documentation describing the structure of the location of stuff within the SDK folder or is it a best guess?

It's just a best guess. It's working now on all platforms, but I don't think there's any guarantee that it works on previous versions or upcoming ones.

michaelnebel · 2023-08-21T11:00:47Z

csharp/extractor/Semmle.Extraction.CSharp.DependencyFetching/Razor.cs

+            foreach (var f in cshtmls.Select(f => f.Replace('\\', '/')))
+            {
+                sw.WriteLine($"\n[{f}]");
+                var base64 = Convert.ToBase64String(Encoding.UTF8.GetBytes(f)); // TODO: this should be the relative path of the file.


Why does it need to be the relative path?

That's what the dotnet CLI is doing. I think this change would have some runtime consequences, but I'm unsure what. Also, it's not something that we currently rely on in any of the checks.

michaelnebel · 2023-08-21T11:08:59Z

csharp/extractor/Semmle.Extraction.CSharp.DependencyFetching/DependencyManager.cs

+                        {
+                            var razor = new Razor(sdk, dotnet, progressMonitor);
+                            razorWorkingDirectory = new TemporaryDirectory(ComputeTempDirectory(sourceDir.FullName, "razor"));
+                            var generatedFiles = razor.GenerateFiles(views, usedReferences.Keys, razorWorkingDirectory.ToString());


nit: consider to use razoeWorkingDirectory.FullName instead of calling ToString().

Just realized that TemporaryDirectory has no FullName. ToString() calls the underlying DirectoryInfo.FullName.

csharp/extractor/Semmle.Extraction.CSharp.DependencyFetching/Razor.cs

michaelnebel · 2023-08-21T11:26:09Z

csharp/ql/integration-tests/all-platforms/cshtml_standalone/Files.expected

@@ -0,0 +1,3 @@
+| Program.cs |
+| Views/Home/Index.cshtml |
+| _semmle_code_target_codeql_csharp_integration_tests_ql_csharp_ql_integration_tests_all_platforms_cshtml_standalone_Views_Home_Index_cshtml.g.cs |


Do we get this long name because this is the name of the integration test folder (+ Views/Home/Index.cshtml)?

michaelnebel · 2023-08-21T11:35:26Z

csharp/tools/tracing-config.lua

@@ -150,6 +150,8 @@ function RegisterExtractorPack(id)
    end

    local windowsMatchers = {
+        CreatePatternMatcher({ '^semmle%.extraction%.csharp%.standalone%.exe$' },


Why is this needed?

We're calling dotnet exec csc, which is a call that is traced by the tracer. We don't want to extract this call, so we need to opt out of it somehow. This lua change makes sure that no processes started by the standalone extractor are traced. This was not working previously on all platforms, so in #13722 there's another implementation as a workaround. But the fix for the MacOS issue is already merged.

csharp/extractor/Semmle.Extraction.CSharp.DependencyFetching/DependencyManager.cs

+                    catch (Exception ex)
+                    {
+                        // It's okay, we tried our best to generate source files from cshtml files.
+                        progressMonitor.LogInfo($"Failed to generate source files from cshtml files: {ex.Message}");
+                    }


tamasvajk requested review from a team as code owners August 14, 2023 14:01

tamasvajk marked this pull request as draft August 14, 2023 14:01

github-actions bot added C# C++ labels Aug 14, 2023

github-code-scanning bot found potential problems Aug 14, 2023

View reviewed changes

csharp/extractor/Semmle.Extraction.CSharp.Standalone/BuildAnalysis.cs Fixed Show fixed Hide fixed

csharp/extractor/Semmle.Extraction.CSharp.Standalone/Razor.cs Fixed Show fixed Hide fixed

github-code-scanning bot found potential problems Aug 14, 2023

View reviewed changes

csharp/extractor/Semmle.Extraction.CSharp.Standalone/BuildAnalysis.cs Fixed Show fixed Hide fixed

tamasvajk commented Aug 14, 2023

View reviewed changes

csharp/extractor/Semmle.Extraction.CSharp.Standalone/Razor.cs Outdated Show resolved Hide resolved

michaelnebel reviewed Aug 15, 2023

View reviewed changes

csharp/extractor/Semmle.Extraction.CSharp.Standalone/DotNet.cs Outdated Show resolved Hide resolved

github-code-scanning bot found potential problems Aug 16, 2023

View reviewed changes

csharp/extractor/Semmle.Extraction.CSharp.Standalone/Razor.cs Fixed Show fixed Hide fixed

tamasvajk force-pushed the razor-standalone-2 branch 3 times, most recently from 6a72e79 to 4ce95b9 Compare August 18, 2023 08:58

github-code-scanning bot found potential problems Aug 18, 2023

View reviewed changes

csharp/extractor/Semmle.Extraction.CSharp.Standalone/BuildAnalysis.cs Fixed Show fixed Hide fixed

tamasvajk force-pushed the razor-standalone-2 branch from 8612891 to 7b6c299 Compare August 18, 2023 09:23

tamasvajk removed the request for review from a team August 18, 2023 09:24

github-actions bot removed the C++ label Aug 18, 2023

tamasvajk marked this pull request as ready for review August 18, 2023 11:54

tamasvajk requested a review from a team August 18, 2023 11:54

michaelnebel self-assigned this Aug 21, 2023

tamasvajk and others added 10 commits August 21, 2023 10:38

C#: Include cshtml files in integration test results

af1a0b9

C#: Add integration test for cshtml extraction in standalone mode

ba0f07b

C#: Generate source files from .cshtml files in standalone

d391246

C#: Run dotnet exec command silently.

d48ab36

C#: Only apply Path.combine to OS agnostic parts of the path.

166633d

Write CSC arguments to file and pass that to the execution

a644133

Add more logging

c2eb2e9

Change backslash to forward slash in file paths in the analyzer confi…

ad3cc8e

…g file

Minor cleanup of dotnet CLI invocations

ba0cc76

Add opt-in environment variable for cshtml generation

84a78e7

tamasvajk added 2 commits August 21, 2023 10:38

Adjust integration test to opt into cshtml extraction

6266691

Fix compile issues after rebase

b5cdaa2

tamasvajk force-pushed the razor-standalone-2 branch from 7b6c299 to b5cdaa2 Compare August 21, 2023 08:43

github-code-scanning bot found potential problems Aug 21, 2023

View reviewed changes

michaelnebel reviewed Aug 21, 2023

View reviewed changes

Improve code quality: fix review findings

2575db3

github-code-scanning bot found potential problems Aug 21, 2023

View reviewed changes

C#: Generate source files from cshtml files in standalone #13957

C#: Generate source files from cshtml files in standalone #13957

tamasvajk commented Aug 14, 2023 •

edited

michaelnebel left a comment

michaelnebel Aug 21, 2023

tamasvajk Aug 21, 2023

michaelnebel Aug 21, 2023

tamasvajk Aug 21, 2023

michaelnebel Aug 21, 2023

tamasvajk Aug 21, 2023

michaelnebel Aug 21, 2023

tamasvajk Aug 21, 2023

michaelnebel Aug 21, 2023

tamasvajk Aug 21, 2023

michaelnebel Aug 21, 2023

tamasvajk Aug 21, 2023

C#: Generate source files from cshtml files in standalone #13957

Are you sure you want to change the base?

C#: Generate source files from cshtml files in standalone #13957

Conversation

tamasvajk commented Aug 14, 2023 • edited

michaelnebel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tamasvajk commented Aug 14, 2023 •

edited