Disallow duplicate directives across #:included files too#54101
Disallow duplicate directives across #:included files too#54101jjonescz wants to merge 3 commits intodotnet:release/10.0.4xxfrom
#:included files too#54101Conversation
Co-authored-by: Copilot <copilot@github.com>
There was a problem hiding this comment.
Pull request overview
Extends file-based app directive validation so duplicate directives are detected across #:include boundaries, providing deterministic failure behavior in multi-file compositions (per #54090).
Changes:
- Adds cross-file duplicate directive detection during virtual project construction (evaluated directives across all included files).
- Updates directive parsing helpers to optionally skip per-file duplicate checks, enabling centralized cross-file deduplication.
- Adds/updates tests to cover duplicate directives across included files for both
dotnet runanddotnet project convert.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/dotnet.Tests/CommandTests/Run/RunFileTests_Directives.cs | New test asserting duplicate directives across included files fail dotnet run with the expected diagnostic. |
| test/dotnet.Tests/CommandTests/Project/Convert/DotnetProjectConvertTests.cs | New conversion test for duplicates across included files; updates diagnostic assertions to include file path. |
| src/Microsoft.DotNet.ProjectTools/VirtualProjectBuilder.cs | Centralizes duplicate detection across all evaluated directives in the include closure. |
| src/Cli/Microsoft.DotNet.FileBasedPrograms/InternalAPI.Unshipped.txt | Updates internal API surface for new optional parameters and the new deduplicator type. |
| src/Cli/Microsoft.DotNet.FileBasedPrograms/FileLevelDirectiveHelpers.cs | Adds checkDuplicates parameter and introduces DirectiveDeduplicator utility used by both parsing and project evaluation. |
Co-authored-by: Copilot <copilot@github.com>
|
|
||
| _seen ??= new(NamedDirectiveComparer.Instance); | ||
|
|
||
| if (_seen.TryGetValue(directive, out var existingDirective)) |
There was a problem hiding this comment.
Should we allow (or warn-and-ignore) identical values? Asking because in C/C++, you can have same #define twice with same value but it errors on different values:
$ printf '%s\n%s' '#define Foo 1' '#define Foo 1' | gcc -xc -Werror - -shared
$ printf '%s\n%s' '#define Foo 1' '#define Foo 1' | clang -xc -Werror - -sharedvs.
$ printf '%s\n%s' '#define Foo 1' '#define Foo 2' | gcc -xc -Werror - -shared
<stdin>:2: error: "Foo" redefined [-Werror]
<stdin>:1: note: this is the location of
cc1: all warnings being treated as errors
$ printf '%s\n%s' '#define Foo 1' '#define Foo 2' | clang -xc -Werror - -shared
<stdin>:2:9: error: 'Foo' macro redefined [-Werror,-Wmacro-redefined]
2 | #define Foo 2
| ^
<stdin>:1:9: note: previous definition is here
1 | #define Foo 1
| ^
1 error generated.There was a problem hiding this comment.
Yes, I think we could do that. This PR doesn't try to change the logic though, it just makes it apply to some unintentionally missed cases. The error for duplicate directives is actually meant to be temporary - until we design how to deduplicate the directives properly. And we can probably relax the restriction incrementally; i.e., I can follow up with your suggestion (allowing duplicate directives if they have the same value too) after this PR.
Related to #54090.