Repository: carbon-language/carbon-lang
File: toolchain/check/eval.cpp (C++)

Code before:
                            "non-facet type {0} combined with `&` operator",
                            SemIR::TypeId);
          // 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.
          // The `arg_id` instruction has no location in it for some reason.
          context.emitter().Emit(
              loc_id, FacetTypeRequiredForTypeAndOperator,
              context.types().GetTypeIdForTypeInstId(type_arg_id));
        }
      }
      // Allow errors to be diagnosed for both sides of the operator before
      // returning here if any error occurred on either side.
      if (!lhs_facet_type_id.has_value() || !rhs_facet_type_id.has_value()) {
        return SemIR::ErrorInst::ConstantId;
      }
      // Reuse one of the argument instructions if nothing has changed.
      if (lhs_facet_type_id == rhs_facet_type_id) {
        return context.types().GetConstantId(
            context.types().GetTypeIdForTypeInstId(arg_ids[0]));
      }
      auto combined_info = SemIR::FacetTypeInfo::Combine(
          context.facet_types().Get(lhs_facet_type_id),
          context.facet_types().Get(rhs_facet_type_id));
      auto const_info =
          GetConstantFacetTypeInfo(eval_context, loc_id, combined_info, &phase);
      return MakeFacetTypeResult(eval_context.context(), const_info, phase);
    }

    case SemIR::BuiltinFunctionKind::IntLiteralMakeType: {
      return context.constant_values().Get(SemIR::IntLiteralType::TypeInstId);
    }

    case SemIR::BuiltinFunctionKind::IntMakeTypeSigned: {
      return MakeIntTypeResult(context, loc_id, SemIR::IntKind::Signed,
                               arg_ids[0], phase);
    }

    case SemIR::BuiltinFunctionKind::IntMakeTypeUnsigned: {
      return MakeIntTypeResult(context, loc_id, SemIR::IntKind::Unsigned,
                               arg_ids[0], phase);
    }

    case SemIR::BuiltinFunctionKind::FloatMakeType: {
      // TODO: Support a symbolic constant width.
      if (phase != Phase::Concrete) {
        break;
      }
      if (!ValidateFloatBitWidth(context, loc_id, arg_ids[0])) {
        return SemIR::ErrorInst::ConstantId;
      }

Review comment (suggestion):
Generally-speaking, I think we should aim to pass each value through `GetConstant*` exactly once during evaluation. I'm not sure how important this is right now -- most of the `GetConstant*` operations are idempotent -- but I think it might matter in some cases: `GetConstantValue` can replace a `BindSymbolicName` with its argument value, and the result of doing so may be another `BindSymbolicName` in some cases, which should then *not* be replaced further.

(From a somewhat more future-looking perspective, we think we'll want to be able to do cross-file constant evaluation eventually, both for computing specifics for imported generics and more generally for evaluating calls to compile-time user-defined functions. My plan for this has been to do the mapping from IDs in one file to IDs in another file as part of the `GetConstantValue` process, so calling it more than once would be problematic for that. That's also motivated trying to make the replacement of instruction operands with their constant values be done as early as possible and as uniformly as possible.)

So I think in this case, because we already `GetConstantValue`'d the function arguments in the general call handling logic (in `MakeConstantForCall`), we shouldn't be doing it again here. Can we keep calling only `ResolveRewriteConstraintsAndCanonicalize` here, or is there more work we need to do? I saw in the main PR comment that this is fixing something for `&` so I assume there's some subtlety here I'm missing, but it looks like the test changes are all either propagating `<error>` more or affecting `where` rather than `&`.

Code after:
                            "non-facet type {0} combined with `&` operator",
                            SemIR::TypeId);
          // 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.
          // The `arg_id` instruction has no location in it for some reason.
          context.emitter().Emit(
              loc_id, FacetTypeRequiredForTypeAndOperator,
              context.types().GetTypeIdForTypeInstId(type_arg_id));
        }
      }
      // Allow errors to be diagnosed for both sides of the operator before
      // returning here if any error occurred on either side.
      if (!lhs_facet_type_id.has_value() || !rhs_facet_type_id.has_value()) {
        return SemIR::ErrorInst::ConstantId;
      }
      // Reuse one of the argument instructions if nothing has changed.
      if (lhs_facet_type_id == rhs_facet_type_id) {
        return context.types().GetConstantId(
            context.types().GetTypeIdForTypeInstId(arg_ids[0]));
      }
      auto combined_info = SemIR::FacetTypeInfo::Combine(
          context.facet_types().Get(lhs_facet_type_id),
          context.facet_types().Get(rhs_facet_type_id));
      if (!ResolveRewriteConstraintsAndCanonicalize(eval_context.context(),
                                                    loc_id, combined_info)) {
        // TODO: Should ResolveRewriteConstraintsAndCanonicalize() move into
        // eval.cpp so it can set the Phase directly?
        phase = Phase::UnknownDueToError;
      }
      return MakeFacetTypeResult(eval_context.context(), combined_info, phase);
    }

    case SemIR::BuiltinFunctionKind::IntLiteralMakeType: {
      return context.constant_values().Get(SemIR::IntLiteralType::TypeInstId);
    }

    case SemIR::BuiltinFunctionKind::IntMakeTypeSigned: {
      return MakeIntTypeResult(context, loc_id, SemIR::IntKind::Signed,
                               arg_ids[0], phase);
    }

    case SemIR::BuiltinFunctionKind::IntMakeTypeUnsigned: {
      return MakeIntTypeResult(context, loc_id, SemIR::IntKind::Unsigned,
                               arg_ids[0], phase);
    }

    case SemIR::BuiltinFunctionKind::FloatMakeType: {
      // TODO: Support a symbolic constant width.
      if (phase != Phase::Concrete) {
        break;