Skip to content

Follow-up to #340: tighten workspace-to-complog support - #352

Merged
jaredpar merged 20 commits into
jaredpar:mainfrom
slang25:slang25/review-pr-340
Sep 2, 2026
Merged

jaredpar merged 20 commits into
jaredpar:mainfrom
slang25:slang25/review-pr-340

Conversation

@slang25

@slang25 slang25 commented May 10, 2026 •

Copy link
Copy Markdown
Contributor

Hi @jaredpar — this is a continuation of #340 (which I've been reviewing). The original PR landed the core of CompilerLogUtil.CreateFromWorkspace / CompilerLogBuilder.AddFromWorkspace; this builds on top of it with a few tweaks I think are worth folding in before merging.

What's in here

  • Non-throwing variant. Adds TryCreateFromWorkspace returning CreateFromWorkspaceResult { Succeeded, CompilerCalls, Diagnostics }. The throwing CreateFromWorkspace is kept and now layers on top of it. AddFromWorkspace returns the synthesized CompilerCall (or null on failure) so callers can correlate diagnostics with projects.
  • TargetFramework recovery. MSBuildWorkspace names multi-targeted projects AssemblyName(tfm), so we parse that first; falls back to the parent directory of OutputFilePath for default-layout SDK projects (best-effort — artifacts/ layouts give {config}_{tfm} and won't round-trip cleanly).
  • Source-generated documents. project.GetSourceGeneratedDocumentsAsync results are now serialized as RawContentKind.GeneratedText and IncludesGeneratedText = true, so the reader uses the captured generator output rather than re-running generators against possibly-different analyzer versions.
  • Project references via the Solution. Switched from CompilationReference to walking project.ProjectReferences and resolving through parentProject.Solution.GetProject(...). Prefers the dep's on-disk OutputFilePath (which matches the consumer's TFM) and only emits the in-memory Compilation when nothing is on disk.
  • Crypto key file. When compilation.Options.CryptoKeyFile is set and resolves to a real file (relative to the project directory if not rooted), it gets captured as RawContentKind.CryptoKeyFile.
  • Smaller polish. ChecksumAlgorithm taken from the first document's SourceText rather than hardcoded SHA-256; ParseOptions read from project.ParseOptions (the authoritative source) rather than from a syntax tree; .vbproj extension fallback for VB projects with no FilePath; cancellation propagated through TryCreateFromWorkspace; the analyzer-skip diagnostic now suggests reloading with BasicAnalyzerKind.OnDisk / None.
  • Tests for round-trip, source-text preservation, project-reference handling, the file-path overload, on-disk analyzer references, generated-text round-trip, cancellation, and a regression lock that the stored args stay empty.

What I tried and backed out

I initially added a WorkspaceCommandLineSynthesizer that reconstructed a csc/vbc-style rsp from compilation.Options + project.MetadataReferences + analyzers (you can see it at b5161e0). The intent was to give complog rsp / replay / export something to work with for workspace-derived complogs.

I hadn't realised that the Roslyn workspace API genuinely doesn't surface emit-time inputs — embedded resources, Win32 manifest/icon/resource, source link, app.config, etc. live on Compilation.Emit() parameters and are supplied by the host (MSBuild), not on Project. So a synthesized rsp would always quietly drop those inputs and produce a binary that compiles but isn't faithful. That's worse than failing loudly, so I removed the synthesizer in 250bd2e and left the stored args empty — replay/rsp/export will fail visibly on workspace-derived complogs, which is the honest behaviour. There's a test that locks in the empty-args invariant.

If you'd rather take the original synthesizer-included version and document the partial fidelity instead, happy to revert that second commit.

Update: merged main in (clean, just a using clash) and pushed a few more fixes from another review pass over the branch:

  • The emitted project-reference cache is now keyed by ProjectId rather than "{AssemblyName}.dll" — assembly names aren't unique across a workspace (each TargetFramework flavour of a multi-targeted project shares one), so the old key could hand one flavour's PE bytes to the other flavour's consumer. There's a regression test locking this in.
  • The serialization core is now properly async (AddFromWorkspaceAsync awaits GetCompilationAsync/GetTextAsync instead of .GetAwaiter().GetResult()), with CreateFromWorkspaceAsync/TryCreateFromWorkspaceAsync exposed and the sync overloads kept as thin wrappers. If that's more public surface than you'd like, happy to trim to just one set — no strong opinion here from me.
  • AddAssembly and the emitted-assembly path now share the zip entry write, ironing out some duplicated logic.
  • The project-reference test no longer depends on the fixture's shape (it used to quietly skip when there were no P2P refs) — it now builds AdhocWorkspace projects directly, exercises the in-memory emit path, and asserts the consumer re-compiles without errors using only what's stored in the log.

🤖 Generated with Claude Code

Copilot AI and others added 9 commits May 10, 2026 11:49
Co-authored-by: jaredpar <146967+jaredpar@users.noreply.github.com>
Introduces CompilerLogUtil.CreateFromWorkspace / TryCreateFromWorkspace
and CompilerLogBuilder.AddFromWorkspace for serializing a Roslyn
workspace's projects directly to a compiler log. The Roslyn workspace
API doesn't surface emit-time inputs (resources, manifests, source
link, etc.), so the stored command line is left empty by default —
replay/rsp/export fail visibly rather than misleadingly.

WorkspaceCommandLineSynthesizer is included for callers willing to
accept the partial-fidelity tradeoff and emit a best-effort rsp from
compilation options.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The synthesizer was retained as a best-effort path for callers willing
to accept partial fidelity, but the Roslyn workspace API genuinely
cannot surface the emit-time inputs needed to round-trip a compilation,
so the synthesized rsp would always be misleading. Drop it; callers
that need an rsp should start from the binary log path instead.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- Key the emitted project-reference cache by ProjectId instead of
  "{AssemblyName}.dll": assembly names are not unique across a workspace
  (each TargetFramework flavor of a multi-targeted project shares one),
  so the old key could hand one flavor's PE bytes to the other's consumer.
- Make the workspace serialization core truly async (AddFromWorkspaceAsync)
  and expose CreateFromWorkspaceAsync / TryCreateFromWorkspaceAsync; the
  sync overloads remain as thin wrappers.
- Share the zip-entry write between AddAssembly and the emitted-assembly
  path via WriteAssemblyEntry, removing the duplicated logic.
- Replace the fixture-dependent project-reference test (which skipped when
  the fixture had no P2P refs) with deterministic AdhocWorkspace-built
  projects that exercise the in-memory emit path and assert the consumer
  re-compiles without errors from the log alone; add a regression test for
  the shared-assembly-name collision.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
TryAddProjectReferenceToDataPackAsync previously recorded a diagnostic and
dropped the reference when the dependency could not be resolved, compiled,
or emitted — leaving TryCreateFromWorkspace reporting Succeeded=true while
the stored compilation silently differed from the workspace one. The
referencing project is now excluded from the log and Succeeded is false,
matching the documented contract.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Conflicts were in the assembly-entry write and the end of
CompilerLogBuilderTests. main routed zip entry creation through the new
CreateEntry helper (fixed timestamp for byte-determinism) while this branch
factored the same write into WriteAssemblyEntry; kept the refactor and moved it
onto CreateEntry so emitted project-reference assemblies get the fixed timestamp
too. The test conflict was two sets of tests appended to the same class.
@codecov

codecov Bot commented Aug 25, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 93.25843% with 30 lines in your changes missing coverage. Please review.
✅ Project coverage is 95.74%. Comparing base (88e9f1b) to head (7dc5c9c).

Files with missing lines Patch % Lines
src/Basic.CompilerLog.Util/CompilerLogBuilder.cs 88.14% 10 Missing and 6 partials ⚠️
...ompilerLog.Util/WorkspaceCommandLineSynthesizer.cs 93.51% 2 Missing and 12 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #352      +/-   ##
==========================================
- Coverage   95.97%   95.74%   -0.24%     
==========================================
  Files          54       56       +2     
  Lines        6163     6601     +438     
  Branches      709      779      +70     
==========================================
+ Hits         5915     6320     +405     
- Misses        130      147      +17     
- Partials      118      134      +16     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Capturing a workspace runs its generators. On .NET Framework the OnDisk
analyzer host loads analyzers with Assembly.LoadFrom into the current AppDomain,
so the test polluted the process and tripped TestBase's assembly load check in
every test running alongside it. Follow the existing RunInContext pattern so the
loads happen in a domain that gets unloaded.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@slang25

slang25 commented Aug 26, 2026

Copy link
Copy Markdown
Contributor Author

@jaredpar what do you think to this feature btw? I'm using this branch at work, and so far it's working for my purposes.

@jaredpar jaredpar left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On one hand I think this PR is on the right track if we're going to support creating from a Workspace. Found a few issues in the PR (see comments) but largely the code is going the right direction.

On the other hand, the lack of fidelity for Workspace is a concerning. You identified that we don't have Emit data and that means it won't be as accurate as a log created from a binary log. To take this we'd need to be onboard with that. Been thinking about that and I think i'm comfortable with it so long as we can get replay / export working. Yes they won't be as high fidelity but it should still be good enough for us to put into the workflows where we consume compiler logs today.

Comment thread src/Basic.CompilerLog.Util/CompilerLogBuilder.cs Outdated
Comment thread src/Basic.CompilerLog.Util/CompilerLogBuilder.cs Outdated
Comment thread src/Basic.CompilerLog.Util/CompilerLogBuilder.cs Outdated
Comment thread src/Basic.CompilerLog.UnitTests/CompilerLogBuilderTests.cs
Comment thread src/Basic.CompilerLog.Util/CompilerLogBuilder.cs
Comment thread src/Basic.CompilerLog.Util/CompilerLogBuilder.cs Outdated
Comment thread src/Basic.CompilerLog.Util/CompilerLogBuilder.cs
Comment thread src/Basic.CompilerLog.Util/CompilerLogBuilder.cs Outdated
Comment thread src/Basic.CompilerLog.Util/CompilerLogBuilder.cs Outdated
Comment thread src/Basic.CompilerLog.Util/CompilerLogUtil.cs Outdated
@jaredpar

Copy link
Copy Markdown
Owner

Sorry, also meant to say in my review thank you for the work and effort you've put into this so far 😄

slang25 and others added 2 commits August 29, 2026 08:14
- Synthesize a best-effort command line for workspace-derived logs so
  rsp / replay / export remain functional, accepting fidelity gaps
  around emit-only inputs the workspace API does not surface
- Add a WorkspaceRoundTrip test over every fixture log: SolutionReader
  -> workspace -> new compiler log -> compile with no errors. Making it
  pass required retaining PE images on reader-materialized references
  (BasicMetadataReference) and replaying stored generated text in the
  None host even when no analyzer references were recorded
- Qualify relative document paths off the project directory
- Re-encode BOM-less non-UTF-8 source text as UTF-8 so it round trips
- Return allProjectReferencesAdded from CreateCompilationDataPackAsync
  instead of mutating a captured local
- Stop writing deprecated HasGeneratedFilesInPdb state
- Cast project.ParseOptions directly, make GetWorkspaceAssemblyFileName
  a local function, assert the stream position contract in
  AddEmittedAssembly, rename CreateFromWorkspaceResult to
  ConvertFromWorkspaceResult

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Comment thread src/Basic.CompilerLog.Util/BasicMetadataReference.cs
slang25 and others added 6 commits August 29, 2026 21:29
AddContentCore now asserts every stored content path is rooted. The
workspace capture path could store relative ones: bare AdhocWorkspace
document names, synthesized source-generated document paths, and the
synthesized project file path itself. Anchor them all to the project
directory, synthesizing a stable rooted directory under the temp path
when the project has no file path.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The compiler casts a module reference's metadata to ModuleMetadata, so
creating AssemblyMetadata unconditionally threw InvalidCastException for
explicit netmodule references (the Windows-only fixture scenarios).
Create the metadata to match the reference kind, capture module-kind
references as netmodules (their image has no assembly manifest), and
synthesize /addmodule: rather than /reference: for them. Adds a
platform-independent regression test that emits a netmodule in memory
and round trips it through the workspace capture.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Codecov flagged the patch at 73.6%: the synthesizer's option branches,
the encoding fallback, the emitted-reference cache, the TargetFramework
fallbacks and the CompilerLogUtil overloads and error paths had no
tests. Adds targeted tests for each. Also switches the synthesizer to
SpecifiedLanguageVersion so /langversion:latest round trips as written
rather than as the mapped concrete version.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
xUnit2031 is an error under CI's -warnaserror build.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Adds direct tests for the Visual Basic option flags (nothing exercised
the On/text sides), the winmdobj/appcontainerexe targets, a concrete
/langversion value, and per-diagnostic severity overrides.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
slang25 added a commit to Buildalyzer/Buildalyzer that referenced this pull request Aug 30, 2026
0.9.54-0.9.60 changed no publicized surface but fixed relative
reference/analyzer paths resolving against the process working
directory instead of the command line's base directory, made log
output deterministic, and asserted the rooted-content-path contract
from jaredpar/complog#387 - which Buildalyzer's inputs already meet.
The round-trip test now also requires an error-free rehydrated
compilation, the bar requested in jaredpar/complog#352 review.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Records a single IsWorkspaceLog flag in the LogInfoPack when a log is
created from a Roslyn workspace, exposed as
ICompilerCallReader.IsWorkspaceLog. The replay, export and rsp commands
print a fidelity warning for such logs: the workspace API doesn't
surface emit-time inputs so their output can differ from the original
build. No metadata version change: older readers skip the extra
MessagePack key and older logs read back false, which is accurate as
they all predate workspace support.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Comment thread src/Basic.CompilerLog.Util/Serialize/MessagePackTypes.cs Outdated
Comment thread src/Basic.CompilerLog.Util/BasicAnalyzerHost.cs Outdated
Comment thread src/Basic.CompilerLog.Util/ICompilerCallReader.cs Outdated
A log can mix compilations created from disk and from a workspace, so
the origin flag moves from LogInfoPack to CompilationInfoPack and
surfaces as CompilerCall.IsWorkspace instead of a reader-level
property. The replay / export / rsp warning now keys off the compiler
calls actually selected by the command line filter, so pulling only
build-derived compilations out of a mixed log doesn't warn. Also
simplifies the zero-analyzer None host path to always replay stored
generated text.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@slang25
slang25 requested a review from jaredpar September 2, 2026 11:53

@jaredpar jaredpar left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution!

@jaredpar
jaredpar merged commit b3df8ac into jaredpar:main Sep 2, 2026
3 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants