Support BitAnd operator between facet types #5022
Conversation
This avoids the suffix changing when adding new impls to the prelude, or in user code.
Doing so results in TODOs in the resulting semir, since we don't handle combining the facet types together properly or doing lookup into them. There's a test added demonstrating this, which will be made to work in followups.
|
This is rebased on top of #5021 |
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
|
@danakj Noting since you requested review again, you might've missed a couple comments |
Oh huh, I did thanks |
toolchain/check/eval.cpp
Outdated
| } else { | ||
| CARBON_DIAGNOSTIC( | ||
| FacetTypeRequiredForTypeAndOperator, Error, | ||
| "BitAnd operator on types requires facet type on both sides"); |
There was a problem hiding this comment.
How about something like:
| "BitAnd operator on types requires facet type on both sides"); | |
| "{0} is not a facet type", SemIR::TypeId); |
(passing context.types().GetTypeIdForTypeInstId(arg_id))
As I think more about the repeated diagnostic -- https://github.com/carbon-language/carbon-lang/blob/trunk/toolchain/docs/diagnostics.md#diagnostic-message-style-guide says "Diagnostics should describe the situation the toolchain observed. The language rule violated can be mentioned if it wouldn't otherwise be clear."
The current phrasing sounds like a language rule, maybe the suggested message is closer to the source of the error? You could add something like "; using & on types requires facet types` if you think it's needed.
There was a problem hiding this comment.
Right true, it's "a non-facet type was found here" error. The downside is that it has to be phrased in the negative with "non-" there. I don't know what name to use for a type that isn't a facet type otherwise.
There was a problem hiding this comment.
This SG, though why aren't you including the type ID?
Note abstractly:
fn GetC() -> type { return C; }
fn GetA() -> type { return A; }
// Should be happy.
({} as C) as (C as (GetA() & GetA()));
// Would be helpful to print `C` here.
({} as C) as (C as (GetC() & GetC()));
There was a problem hiding this comment.
Ah that's a good idea, I hadn't thought of it.
+ // CHECK:STDERR: fail_combine_with_non_facet_type.carbon:[[@LINE+4]]:23: error: non-facet type `C` combined with `&` operator [FacetTypeRequiredForTypeAndOperator]
That's better :) Thanks!
toolchain/check/eval.cpp
Outdated
| // TODO: Find a location for the lhs or rhs specifically, instead of | ||
| // the whole thing. If that's not possible we can change the text to | ||
| // say if it's referring to the left or the right side for the error. | ||
| context.emitter().Emit(loc, FacetTypeRequiredForTypeAndOperator); |
There was a problem hiding this comment.
Instead of loc, can you pass arg_id here?
There was a problem hiding this comment.
I think I preemptively answered this - but no. The arg_id has no location at all, so the diagnostics get orphaned at the top of the test output.
There was a problem hiding this comment.
FTR this is weird to me, I think the instruction should have a location. But that's not for this PR.
toolchain/check/eval.cpp
Outdated
| // TODO: Find a location for the lhs or rhs specifically, instead of | ||
| // the whole thing. If that's not possible we can change the text to | ||
| // say if it's referring to the left or the right side for the error. | ||
| context.emitter().Emit(loc, FacetTypeRequiredForTypeAndOperator); |
There was a problem hiding this comment.
I think I preemptively answered this - but no. The arg_id has no location at all, so the diagnostics get orphaned at the top of the test output.
toolchain/check/eval.cpp
Outdated
| } else { | ||
| CARBON_DIAGNOSTIC( | ||
| FacetTypeRequiredForTypeAndOperator, Error, | ||
| "BitAnd operator on types requires facet type on both sides"); |
There was a problem hiding this comment.
Right true, it's "a non-facet type was found here" error. The downside is that it has to be phrased in the negative with "non-" there. I don't know what name to use for a type that isn't a facet type otherwise.
toolchain/check/eval.cpp
Outdated
| // TODO: Find a location for the lhs or rhs specifically, instead of | ||
| // the whole thing. If that's not possible we can change the text to | ||
| // say if it's referring to the left or the right side for the error. | ||
| context.emitter().Emit(loc, FacetTypeRequiredForTypeAndOperator); |
There was a problem hiding this comment.
FTR this is weird to me, I think the instruction should have a location. But that's not for this PR.
toolchain/check/eval.cpp
Outdated
| } else { | ||
| CARBON_DIAGNOSTIC( | ||
| FacetTypeRequiredForTypeAndOperator, Error, | ||
| "BitAnd operator on types requires facet type on both sides"); |
There was a problem hiding this comment.
This SG, though why aren't you including the type ID?
Note abstractly:
fn GetC() -> type { return C; }
fn GetA() -> type { return A; }
// Should be happy.
({} as C) as (C as (GetA() & GetA()));
// Would be helpful to print `C` here.
({} as C) as (C as (GetC() & GetC()));
Doing so results in TODOs in the resulting semir, since we don't handle
combining the facet types together properly or doing lookup into them.
There's a test added demonstrating this, which will be made to work in
followups.