[msbuild-quality] MSBuild shipped targets: missing FileWrites, stale fallback, DependsOn overwrite

### 🔧 MSBuild File Quality Report — 2026-08-26

**Files reviewed**: 20 (all category-1 shipped SDK build logic + VS shims)
**Findings**: 🔴 1 error · 🟡 2 warnings · 🔵 1 suggestion

---

### 🔴 Errors

#### `src/FSharp.Build/Microsoft.FSharp.Targets` — Rule A-1: `CreateManifestResourceNamesDependsOn` overwrite
- **Line**: 123
- **Current**: ``
- **Issue**: Sets the property to empty, dropping any value a prior import may have appended. If any SDK target or NuGet package adds to `CreateManifestResourceNamesDependsOn`, that dependency is silently lost.
- **Suggested**: `$(CreateManifestResourceNamesDependsOn)` — or add a comment explaining why the overwrite is intentional (F# replaces the SDK’s `CreateManifestResourceNames` entirely).

> **Note**: The `CoreCompileDependsOn` assignment at line 224 (`_ComputeNonExistentFileProperty`) looks like the same pattern, but is safe because F# defines its own `CoreCompile` target (line 280) that replaces the C# SDK’s, and the property is read at execution time after `Microsoft.Common.targets` (imported at line 430) has a chance to contribute. Still, a brief comment would help future readers.

### 🟡 Warnings

#### `src/FSharp.Build/Microsoft.FSharp.NetSdk.targets` — Rule A-4: Missing `FileWrites` for ILLink substitutions
- **Line**: 213–219 (`GenerateFSharpILLinkSubstitutions` target)
- **Current**: The `GenerateILLinkSubstitutions` task outputs items to `@(EmbeddedResource)` but does **not** register the generated file in `@(FileWrites)`.
- **Impact**: `dotnet clean` won’t remove the generated `ILLink.Substitutions.xml` from `obj/`.
- **Suggested**: Add `` (or a subsequent ``).

#### `vsintegration/shims/Microsoft.FSharp.ShimHelpers.props` — Stale TODO / hardcoded fallback
- **Line**: 35–38
- **Current**:
```xml


<_FSCorePackageVersionSet>true
6.0.4

```
- **Issue**: The comment says “Remove before shipping” but the block is still present and pins a hardcoded `6.0.4` version as a fallback. This was added as a temporary workaround; if it’s now permanent, the comment should be updated. If it’s truly obsolete, it should be removed.

### 🔵 Suggestions

#### `src/FSharp.Build/Microsoft.FSharp.Targets` — Add explanatory comments on intentional DependsOn overwrites
- **Lines**: 123, 224
- **Suggestion**: Both `CreateManifestResourceNamesDependsOn` and `CoreCompileDependsOn` intentionally override the SDK’s chain because F# replaces those targets entirely. A one-line comment (e.g., ``) would prevent future reviewers from filing this as a bug.

Files reviewed (no issues found)

- `src/FSharp.Build/Microsoft.FSharp.NetSdk.props` — Good condition guards, proper semicolon-list composition for `WarningsAsErrors`, `NoWarn`, `DefineConstants`
- `src/FSharp.Build/Microsoft.FSharp.Core.NetSdk.props` — Clean sentinel-guarded import pattern
- `src/FSharp.Build/Microsoft.FSharp.Overrides.NetSdk.targets` — Good `FileWrites` registration for generated assembly info
- `src/FSharp.Build/Microsoft.Portable.FSharp.Targets` — Good `Exists()` guards on all imports
- `src/fsc/fsc.targets` — Proper `$(NoWarn)` composition
- `src/fsi/fsi.targets` — Proper `$(NoWarn)` and `$(DefineConstants)` composition
- `vsintegration/shims/Microsoft.FSharp.NetSdk.Shim.props` — Clean
- `vsintegration/shims/Microsoft.FSharp.NetSdk.Shim.targets` — Clean
- `vsintegration/shims/Microsoft.FSharp.Overrides.NetSdk.Shim.targets` — Clean
- `vsintegration/shims/Microsoft.FSharp.Shim.targets` — Clean
- `vsintegration/shims/Microsoft.Portable.FSharp.Shim.targets` — Clean
- `vsintegration/Vsix/VisualFSharpFull/VisualFSharp.Core.targets` — Clean (project references, no build logic)
- `src/FSharp.Build/Directory.Build.props`, `src/fsc/Directory.Build.props`, `src/fsi/Directory.Build.props`, `vsintegration/Vsix/Directory.Build.props`, `vsintegration/Vsix/Directory.Build.targets` — Infrastructure, clean

---

### Review Rules Reference

This review checks against MSBuild canonical patterns for:
- **Target authoring**: DependsOn chains, Returns vs Outputs, incremental build, FileWrites
- **Property patterns**: Conditional defaults, quoted conditions, semicolon composition, path normalization
- **Item management**: Include/Remove/Update, batching, generated file placement
- **Extension points**: Import guards, CustomBefore/After hooks, cross-platform paths

*Generated by [MSBuild Quality Review](https://github.com/dotnet/fsharp/actions/runs/32932761957)*

> Generated by [F# MSBuild File Quality Review Agent](https://github.com/dotnet/fsharp/actions/runs/32932761957) · opus46 3.8M · [◷](https://github.com/search?q=repo%3Adotnet%2Ffsharp+is%3Aissue+%22gh-aw-workflow-call-id%3A+dotnet%2Ffsharp%2Fmsbuild-quality-review%22&type=issues)

Inspect src/FSharp.Build/Microsoft.FSharp.Targets around lines 123 and 224, src/FSharp.Build/Microsoft.FSharp.NetSdk.targets around lines 213–219, and vsintegration/shims/Microsoft.FSharp.ShimHelpers.props around lines 35–38. Start by tracing the affected MSBuild properties and generated items, then use the repository’s build and clean workflows to verify the intentional overrides remain valid, generated ILLink output is cleaned, and the fallback comment or block is resolved.

Written by the indexing model from the issue text.