Skip to content

Format the call parameters of a function, not the patterns. #5342

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

zygoloid
Copy link
Contributor

This makes the parameters printed in a SemIR fn declaration match the arguments printed in a SemIR call instruction.

This makes the parameters printed in a SemIR `fn` declaration match the
arguments printed in a SemIR `call` instruction.
@github-actions github-actions bot requested a review from dwblaikie April 21, 2025 23:37
@zygoloid zygoloid marked this pull request as draft April 22, 2025 00:03
@zygoloid zygoloid removed the request for review from dwblaikie April 22, 2025 00:04
Copy link
Contributor

@geoffromer geoffromer left a comment

Choose a reason for hiding this comment

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

(I'm about halfway through the testdata changes)

@@ -101,7 +101,7 @@ fn H() { G(3); }
// CHECK:STDOUT: %array_type.loc9_24.2: type = array_type %int.convert_checked.loc9_23.2, constants.%i32 [symbolic = %array_type.loc9_24.2 (constants.%array_type)]
// CHECK:STDOUT: %require_complete: <witness> = require_complete_type %array_type.loc9_24.2 [symbolic = %require_complete (constants.%require_complete.7cb)]
// CHECK:STDOUT:
// CHECK:STDOUT: fn(%N.patt.loc4_6.1: %i32) {
// CHECK:STDOUT: fn() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it expected that the param goes away entirely for N:! i32?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes; the idea is that the parameter list shown here corresponds to the args that are passed to the call inst. That excludes symbolic parameters, because they aren't passed at run-time.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I see it's still listed in the @F name on line 93, so that's good. Otherwise overloads that differ on compttime parameters will be confusing to read. I think it's a bit weird that this differs but you have an intentional reason for doing so and will try to remember what this means.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I assume when we support overloading, we'll have some way to uniquely identify individual overloads in the IR, and we'll use that in place of @F (or whatever).

@@ -43,9 +43,9 @@ fn Boo[a: ()]() {}
// CHECK:STDOUT: %Boo.decl: %Boo.type = fn_decl @Boo [concrete = constants.%Boo] {} {}
// CHECK:STDOUT: }
// CHECK:STDOUT:
// CHECK:STDOUT: fn @Foo[<error>: <error>]();
// CHECK:STDOUT: fn @Foo();
Copy link
Contributor

Choose a reason for hiding this comment

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

Dropping errors from the output seems a bit confusing, can they be preserved?

Copy link
Contributor

Choose a reason for hiding this comment

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

If the absence of the errors is a bug, it's not a new bug here, it's a pre-existing bug on line 42, which doesn't show any errors in the declaration of Foo.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, yes, it would be nice to preserve errors there too.

@zygoloid zygoloid marked this pull request as ready for review April 22, 2025 18:09
@github-actions github-actions bot requested a review from danakj April 22, 2025 18:09
Copy link
Contributor

@danakj danakj left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -43,9 +43,9 @@ fn Boo[a: ()]() {}
// CHECK:STDOUT: %Boo.decl: %Boo.type = fn_decl @Boo [concrete = constants.%Boo] {} {}
// CHECK:STDOUT: }
// CHECK:STDOUT:
// CHECK:STDOUT: fn @Foo[<error>: <error>]();
// CHECK:STDOUT: fn @Foo();
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, yes, it would be nice to preserve errors there too.

@@ -101,7 +101,7 @@ fn H() { G(3); }
// CHECK:STDOUT: %array_type.loc9_24.2: type = array_type %int.convert_checked.loc9_23.2, constants.%i32 [symbolic = %array_type.loc9_24.2 (constants.%array_type)]
// CHECK:STDOUT: %require_complete: <witness> = require_complete_type %array_type.loc9_24.2 [symbolic = %require_complete (constants.%require_complete.7cb)]
// CHECK:STDOUT:
// CHECK:STDOUT: fn(%N.patt.loc4_6.1: %i32) {
// CHECK:STDOUT: fn() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I see it's still listed in the @F name on line 93, so that's good. Otherwise overloads that differ on compttime parameters will be confusing to read. I think it's a bit weird that this differs but you have an intentional reason for doing so and will try to remember what this means.

@geoffromer geoffromer enabled auto-merge April 22, 2025 18:36
@geoffromer geoffromer added this pull request to the merge queue Apr 22, 2025
Merged via the queue into carbon-language:trunk with commit ca8df34 Apr 22, 2025
8 checks passed
chandlerc pushed a commit to chandlerc/carbon-lang that referenced this pull request Apr 25, 2025
…anguage#5342)

This makes the parameters printed in a SemIR `fn` declaration match the
arguments printed in a SemIR `call` instruction.
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.

3 participants