Skip to content

Conversation

danakj
Copy link
Contributor

@danakj danakj commented Jun 20, 2025

The full.carbon prelude just sets a flag indicating an explicit intent to include the full prelude. Once all tests include some prelude file, an error can be enabled (currently it's commented out) that requires an INCLUDE-FILE of some min-prelude to be present in all check/ and lower/ file tests.

@github-actions github-actions bot requested a review from dwblaikie June 20, 2025 18:37
Copy link
Contributor

@dwblaikie dwblaikie left a comment

Choose a reason for hiding this comment

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

I get the impression, though haven't looked closely, that file_test is pretty well tested independently of the specific carbon use case where possible - so maybe it'd be possible/valuable to split this patch in two - one to implement the new extra args support including generic tests for that (it's possibly I'm speculating too rampantly and it's not practical to do this... ), and then separately support for... actually I guess there's two steps here - support+tests for expect-full-prelude, and support+tests for multiple exclude-dump-file-prefixes?

Comment on lines 513 to 518
for (auto prefix : options_->exclude_dump_file_prefixes) {
if (unit->input_filename().starts_with(prefix)) {
return false;
}
}
return true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could write this as:

return llvm::none_of(options_->exclude_dump_file_prefixes, [&](auto prefix) { return unit->inut_filename().starts_with(prefix); });

I sometimes find the predicate form a bit more readable, but it's certainly marginal/debateable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh that's nice, yeah, thanks. Done.

Note this is tested by toolchain/lower/testdata/operators/arithmetic.carbon using the full.carbon prelude, which then has 2 paths that need to be excluded in semir. If this is wrong, we get more (unwanted) semir output in the test. For example:

+// CHECK:STDOUT: ; ModuleID = 'min_prelude/full.carbon'
+// CHECK:STDOUT: source_filename = "min_prelude/full.carbon"
+// CHECK:STDOUT:
+// CHECK:STDOUT: !llvm.module.flags = !{!0, !1}
+// CHECK:STDOUT: !llvm.dbg.cu = !{!2}
...

Or:

+// CHECK:STDOUT: ; ModuleID = '/private/var/tmp/_bazel_danakj/1c85fcb7c35d2ecb3905beb34bfff611/execroot/_main/bazel-out/darwin_arm64-dbg/bin
/toolchain/install/prefix_root/lib/carbon/../../lib/carbon/core/io.carbon'
+// CHECK:STDOUT: source_filename = "/private/var/tmp/_bazel_danakj/1c85fcb7c35d2ecb3905beb34bfff611/execroot/_main/bazel-out/darwin_arm64-db
g/bin/toolchain/install/prefix_root/lib/carbon/../../lib/carbon/core/io.carbon"
+// CHECK:STDOUT:
+// CHECK:STDOUT: define i32 @_CEOF.Core() !dbg !4 {
+// CHECK:STDOUT: entry:
+// CHECK:STDOUT:   ret i32 -1, !dbg !7
+// CHECK:STDOUT: }
...

@danakj
Copy link
Contributor Author

danakj commented Jun 23, 2025

I get the impression, though haven't looked closely, that file_test is pretty well tested independently of the specific carbon use case where possible - so maybe it'd be possible/valuable to split this patch in two - one to implement the new extra args support including generic tests for that (it's possibly I'm speculating too rampantly and it's not practical to do this... ), and then separately support for... actually I guess there's two steps here - support+tests for expect-full-prelude, and support+tests for multiple exclude-dump-file-prefixes?

Oh I will have to look, I haven't really poked at tests of file_test before. I can't test expect-full-prelude itself in this PR really since it does nothing yet, until we get all tests using a min-prelude, though I test that it does nothing successfully by including it in one test file.

@danakj
Copy link
Contributor Author

danakj commented Jun 23, 2025

I get the impression, though haven't looked closely, that file_test is pretty well tested independently of the specific carbon use case where possible - so maybe it'd be possible/valuable to split this patch in two - one to implement the new extra args support including generic tests for that

I guess you're thinking of //testing/file_test tests, but these changes are not general for file_test as a testing platform, they are specific to the toolchain's implementation of file_test (changes are all under //toolchain/testing/, preludes are a toolchain-specific concept). We don't have tests at that level, and the file tests themselves are themselves mostly tests of it - the semir output being what we expect requires the prelude inclusion, and path exclusion, to be correct. When they are wrong, the semir output changes.

The one thing not tested is that we make an error when no prelude flag is passed, but we don't do that yet. I can instantiate and mock out parts of ToolchainFileTest but that will require a fairly large refactoring. How much risk do you think there is of the error stopping working and us not noticing?

@danakj danakj requested a review from dwblaikie June 23, 2025 16:00
@dwblaikie
Copy link
Contributor

I get the impression, though haven't looked closely, that file_test is pretty well tested independently of the specific carbon use case where possible - so maybe it'd be possible/valuable to split this patch in two - one to implement the new extra args support including generic tests for that

I guess you're thinking of //testing/file_test tests, but these changes are not general for file_test as a testing platform, they are specific to the toolchain's implementation of file_test (changes are all under //toolchain/testing/, preludes are a toolchain-specific concept). We don't have tests at that level, and the file tests themselves are themselves mostly tests of it - the semir output being what we expect requires the prelude inclusion, and path exclusion, to be correct. When they are wrong, the semir output changes.

Ah, yep. Makes sense - thanks for looking/explaining and sorry for the dead end suggestion.

@dwblaikie dwblaikie added this pull request to the merge queue Jun 24, 2025
Merged via the queue into carbon-language:trunk with commit badd544 Jun 24, 2025
8 checks passed
@danakj danakj deleted the full-prelude branch June 26, 2025 16:05
github-merge-queue bot pushed a commit that referenced this pull request Aug 15, 2025
Trying to figure out an easy way to debug semir in the prelude, #5703
removed an option to set `--exclude-dump-file-prefix` to empty. But,
this is probably an improvement over that flow... With this change, it's
possible to add `//@dump-sem-ir-file` to a specific prelude file, and
its full IR will be printed. Additionally, it becomes an option with the
default `--dump-sem-ir-ranges=only` to add `//@dump-sem-ir-file` and get
the full file's IR.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants