Skip to content
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

Support user-defined implicit conversions via ImplicitAs #1273

Merged
merged 31 commits into from
May 21, 2022
Merged

Support user-defined implicit conversions via ImplicitAs #1273

merged 31 commits into from
May 21, 2022

Conversation

zygoloid
Copy link
Contributor

@zygoloid zygoloid commented May 17, 2022

Support is added for all of the non-type expression contexts where we currently accept built-in conversions, such as reordering the fields in a struct.

Prepend the generic class's list of parameters onto the `impl`'s list of
deduced parameters, and add the `impl` to the impl scope enclosing the
class rather than adding it to the impl scope for the class body.

When matching an impl, deduce against both the type and the interface.
Add `ValueLiteral` to represent a type-checking-generated value.
… and a function to actually type-check the expression.
Implicit conversion support for calls.
Add test for implicit conversion in function calls now that it works.
@zygoloid zygoloid added explorer Action items related to Carbon explorer code DO NOT MERGE labels May 17, 2022
@zygoloid zygoloid requested a review from a team as a code owner May 17, 2022 21:18
Fix bug exposed by this where choice values and patterns were given the
wrong type -- the type of a choice expression was the type of the choice
type instead of the choice type itself.
For now, require that all patterns match the same type. This type can
be different from the type of the scrutinee, in which case it will be
converted, but it will only be converted once, before all matches are
performed. This behavior should be revisited once we have decided on
detailed rules for `match`.

Simplify now that all pattern matching performs implicit conversions.
…operands and a function to actually type-check the expression."

Instead, have TypeCheckExp bail out if it sees an expression that's
already been type-checked. As suggested by @jonmeow in #1277.
Copy link
Contributor

@jonmeow jonmeow 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 happy to do another round before end of day, but ping me directly since I'm probably vanishing for a couple weeks around 4pm.

explorer/ast/expression.h Outdated Show resolved Hide resolved
Comment on lines +104 to +105
// Can only be called by type-checking, if a conversion was required.
void set_rhs(Nonnull<Expression*> rhs) { rhs_ = rhs; }
Copy link
Contributor

Choose a reason for hiding this comment

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

Where you're adding setters, is there anything this can do relative to checking type checking of rhs_ or rhs that would help enforce the "Can only"? Maybe it would make sense to do something like "set_desugared_rhs" which could only be called once, and a desugared_rhs getter that returns desugared_rhs if set, rhs otherwise?

Note, I'm wondering about this mainly in the larger context of adding setters. This could go a step further with a templated type, e.g.:

template <typename T>
class Desugarable {
 public:
  Desugarable(Nonnull<T*> original) : original_ {}
  // Should be called at most once by type-checking.
  void set_desugared(Nonnull<T*> desugared) { CARBON_CHECK(!desugarded_); desugared_ = desugarded; }

  // Would it even work to have the non-const version CARBON_CHECK if desugared_ is already set, in order to prevent accidents?
  auto original() -> T& { return original_; }
  auto original() const -> const T& { return original_; }
  auto desugared() const -> const T& { return desugared_ ? *desugarded_ : original_; }

 private:
  Nonnull<T*> original_;
  std::optional<Nonnull<T*>> desugarded_;
};

Then maybe something like this becomes:

auto rhs() const -> const Desugurable<Expression>& { return rhs_; }
auto rhs() -> Desugurable<Expression>& { return rhs_; }
Desugarable<Expression> rhs_;

However, that's a much higher code delta, especially because it affects interpreter call sites. Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think there is probably a better direction here, but I think it's likely to take a bit of discussion to hammer out the best approach. Do you want this addressed before this change lands, or can this be deferred to a later refactoring?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's probably reasonable to defer to a later refactoring, that was part of my concern over size of the delta.

fn Convert[me: Self]() -> T;
}

// FIXME: Simplify this once we have variadics.
Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW, I'd typically suggest TODO instead of FIXME for anything that's expected to outlast the given PR, because that's what the style guide says to use:
https://google.github.io/styleguide/cppguide.html#TODO_Comments

I haven't commented much on this because I'm not sure how much you're using FIXME to indicate things you intend to fix either before merging or quickly after, but this sounds like it's very much "eventually".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just to note, I'm doing more cleanup of this in #1282

explorer/interpreter/builtins.h Outdated Show resolved Hide resolved
Comment on lines 20 to 26
impl forall [U1:! Type, U2:! Type, T1:! ImplicitAs(U1), T2:! ImplicitAs(U2)] (T1, T2) as ImplicitAs((U1, U2)) {
fn Convert[me: Self]() -> (U1, U2) {
let (v1: T1, v2: T2) = me;
return (v1.Convert(), v2.Convert());
}
}
impl forall [U1:! Type, U2:! Type, U3:! Type, T1:! ImplicitAs(U1), T2:! ImplicitAs(U2), T3:! ImplicitAs(U3)] (T1, T2, T3) as ImplicitAs((U1, U2, U3)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should probably wrap these to 80 char columns

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, though I somewhat arbitrarily invented some wrapping and indent rules. I assume we'll eventually have a tool to automate this, and we'll run it over all of our existing Carbon code once it exists, so I guess the precise formatting choices right now don't matter too much.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, arbitrary rules is kind of what I expected.

explorer/testdata/match/convert.carbon Outdated Show resolved Hide resolved
explorer/testdata/match/fail_pattern_type_mismatch.carbon Outdated Show resolved Hide resolved
explorer/interpreter/type_checker.h Outdated Show resolved Hide resolved
explorer/interpreter/type_checker.cpp Outdated Show resolved Hide resolved
explorer/interpreter/type_checker.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@jonmeow jonmeow left a comment

Choose a reason for hiding this comment

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

Approving -- as noted, I think it's fine to refactor the desugaring further in another PR.

@zygoloid zygoloid merged commit af04288 into carbon-language:trunk May 21, 2022
chandlerc pushed a commit that referenced this pull request Jun 28, 2022
Support is added for all of the non-type expression contexts where we currently accept built-in conversions, such as reordering the fields in a struct.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
explorer Action items related to Carbon explorer code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants