Reduce use of the prelude in tests #5683

danakj · 2025-06-17T16:30:36Z

Remove use of i32/bool when a builtin type or test-define class type can work. Make Sub user-defines in a test that is testing builtin functions and not trying to test the prelude, in the same way that it defines its own Negate. Reduce use of the + operator when it isn't contributing to the test's coverage, since the + operator needs the full prelude. Remove use of Core.Print when it's not required for the test.

Move deduce_nested_facet_value.carbon to its own file since it uses TypeAnd, and the rest of deduce.carbon does not, but uses i32. This means they can each use a different min-prelude.

danakj · 2025-06-17T16:31:09Z

This is a prereq for #5676 #5680 #5681 and #5682. They will be rebased over it.

jonmeow

Thanks for splitting this out, sorry for being a pain about it.

jonmeow · 2025-06-17T17:16:58Z

toolchain/check/testdata/builtins/int/make_type_signed.carbon

 import library "types";

-fn Negate(n: i32) -> i32 = "int.snegate";
+fn Negate(a: IntLiteral()) -> IntLiteral() = "int.snegate";


Suggested change

fn Negate(a: IntLiteral()) -> IntLiteral() = "int.snegate";

fn Negate(a: Int(32)) -> Int(32) = "int.snegate";

Can this more precisely match the prior i32, or does it specifically need to be IntLiteral to avoid a conversion?

The old thing was an oversight, we only need to negate literals here. This is more obvious in snegate, where using Int(32) fails with the inputs, since they are larger values. Using Int(32) here instead adds a dependency that the test doesn't need.

jonmeow · 2025-06-17T17:22:03Z

toolchain/check/testdata/class/generic/redeclare.carbon

 library "[[@TEST_NAME]]";

-class B(N:! i32);
+class I {}


Sorry, small thing. Can you choose something like class A or class Other? I in particular I automatically think "interface".

I was already thinking about this looking at fail_generic_method.carbon, but this file uses the pattern multiple times. :)

jonmeow · 2025-06-17T17:25:10Z

toolchain/check/testdata/function/generic/deduce_nested_facet_value.carbon

+// TIP: To dump output, run:
+// TIP:   bazel run //toolchain/testing:file_test -- --dump_output --file_tests=toolchain/check/testdata/function/generic/deduce_nested_facet_value.carbon
+
+// --- deduce_nested_facet_value.carbon


FWIW -- to the extent that this is about performance of prelude parsing, splitting out tests shouldn't be expected to make tests faster since it's a per-file cost (actually, it'll probably marginally slow down due to the min_prelude + full prelude use instead of just full prelude).

I'd suggest leaving this where it belongs logically, and choose min_prelude based on what works, rather than using min_prelude as a primary rationale for splits.

There's no min_prelude that works for both, so we'd end up with a full prelude - that's why it's moving. The deduce file needs int.carbon as a prelude and it doesn't have TypeAnd. This test needs facet_types.carbon, and it doesn't have i32.

jonmeow · 2025-06-17T17:25:57Z

toolchain/check/testdata/impl/use_assoc_const.carbon

@@ -20,7 +20,7 @@ interface J {
 }

 impl () as J where .U = i32 {
-  fn F[self: Self](u: i32) -> i32 { return u + 2; }
+  fn F[self: Self](u: i32) -> i32 { return u; }


Was this possibly validating that it could make use of the + operator on i32? (i.e., doing some minor impls validation)

Given min_prelude, maybe switch to -u, which still requires an operator lookup?

Suggested change

fn F[self: Self](u: i32) -> i32 { return u; }

fn F[self: Self](u: i32) -> i32 { return -u; }

I don't think it was, this was testing the ability to call the associated function. But returning something different than it received is fine, sure.

jonmeow · 2025-06-17T17:26:46Z

toolchain/check/testdata/impl/use_assoc_const.carbon

@@ -47,7 +47,7 @@ interface J {
 class D { }

 impl D as J where .U = i32 {
-  fn F(u: i32) -> i32 { return u + 3; }
+  fn F(u: i32) -> i32 { return u; }


Suggested change

fn F(u: i32) -> i32 { return u; }

fn F(u: i32) -> i32 { return -u; }

jonmeow · 2025-06-17T17:27:21Z

toolchain/check/testdata/impl/use_assoc_const.carbon

@@ -66,16 +66,20 @@ interface J {
 class E {
  extend impl as J where .U = i32 {
    fn F(u: i32) -> i32 {
-      return u + 1;
+      return u;


Suggested change

return u;

return -u;

jonmeow · 2025-06-17T17:27:29Z

toolchain/check/testdata/impl/use_assoc_const.carbon

    }
    fn G[self: Self](v: i32) -> i32 {
-      return v + 2;
+      return v;


Suggested change

return v;

return -v;

jonmeow · 2025-06-17T17:28:47Z

toolchain/lower/testdata/function/generic/call_impl_function.carbon

 impl ImplsSomeInterface as SomeInterface(i32) {
  fn F(x: i32) -> i32 {
-    return x + x;
+    return x;


Suggested change

return x;

return -x;

(see other comment about trying to use impls of i32)

jonmeow · 2025-06-17T17:31:44Z

toolchain/lower/testdata/function/generic/cross_library_name_collision_private.carbon

+// TODO: This should call the `F` from lib1 and the `F` from lib2, where one
+// returns 1 and the other returns 2. Currently it calls the same function
+// twice, both returning 1.


I'll just note, in theory the previous "return 3" comment could be compiled and verified. Now that's not really the case. But the "returns 1" also can't be validated AFAICT, I don't think it's really obvious from the code.

Personally, I'd suggest something more like:

Suggested change

// TODO: This should call the `F` from lib1 and the `F` from lib2, where one

// returns 1 and the other returns 2. Currently it calls the same function

// twice, both returning 1.

// TODO: This should call the `F` from lib1 and the `F` from lib2. Currently

// it calls the same function in both (hashes match for both instances of

// `call i32 @_CF.Main.<hash>`).

Yeah, it's only observable/validated by reading the lower output. Thanks, done.

Co-authored-by: Jon Ross-Perkins <[email protected]>

reduce-prelude

41b60ba

danakj requested a review from jonmeow June 17, 2025 16:30

github-actions bot added the toolchain label Jun 17, 2025

github-actions bot requested a review from geoffromer June 17, 2025 16:31

danakj mentioned this pull request Jun 17, 2025

Make more tests into min-prelude #5676

Merged

jonmeow approved these changes Jun 17, 2025

View reviewed changes

danakj added 3 commits June 17, 2025 13:38

class-i

a90caa9

Merge branch 'trunk' into reduce-prelude

92b3c1f

i-to-a

397f589

danakj force-pushed the reduce-prelude branch from eebe126 to 397f589 Compare June 17, 2025 17:41

danakj and others added 2 commits June 17, 2025 13:48

negates

0f1d1ce

todo-wording

3b9c807

Co-authored-by: Jon Ross-Perkins <[email protected]>

danakj enabled auto-merge June 17, 2025 17:50

danakj added this pull request to the merge queue Jun 17, 2025

Merged via the queue into carbon-language:trunk with commit 19d59b2 Jun 17, 2025
8 checks passed

danakj deleted the reduce-prelude branch June 17, 2025 18:10

danakj mentioned this pull request Jun 17, 2025

Add min-preludes to most of lowering and a few more slow tests #5680

Merged

	fn Negate(a: IntLiteral()) -> IntLiteral() = "int.snegate";
	fn Negate(a: Int(32)) -> Int(32) = "int.snegate";

	fn F[self: Self](u: i32) -> i32 { return u; }
	fn F[self: Self](u: i32) -> i32 { return -u; }

	fn F(u: i32) -> i32 { return u; }
	fn F(u: i32) -> i32 { return -u; }

Reduce use of the prelude in tests #5683

Reduce use of the prelude in tests #5683

Uh oh!

Conversation

danakj commented Jun 17, 2025

Uh oh!

danakj commented Jun 17, 2025

Uh oh!

jonmeow left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonmeow Jun 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jonmeow Jun 17, 2025 •

edited

Loading