Feature generic choice #2042

pmqtt · 2022-08-16T09:47:18Z

Implementation of generic choices!

I hope it is useful!

kmz42

Added a few thoughts. Still new to the project so feel free to tell me i'm wrong. Nice PR 🎉

kmz42 · 2022-08-16T13:31:32Z

explorer/interpreter/value.h

@@ -838,7 +838,26 @@ class ChoiceType : public Value {
  ChoiceType(std::string name, std::vector<NamedValue> alternatives)
      : Value(Kind::ChoiceType),
        name_(std::move(name)),
-        alternatives_(std::move(alternatives)) {}
+        alternatives_(std::move(alternatives)),
+        is_generic_(false) {}


Do we need is_generic_? Isn't it implied by the presence or absence of bindings (as with NominalClassType)?

Yes, we need is_generic! NominalClassType can check over type_args that binding is present or not.

kmz42 · 2022-08-16T13:38:26Z

explorer/interpreter/value.h

+        alternatives_(std::move(alternatives)),
+        is_generic_(false) {}
+
+  ChoiceType(std::string name, std::vector<NamedValue> alternatives,


maybe i'm missing context but why do we even have a ChoiceType constructor that uses std::string name instead of only a ChoiceDeclaration?

It looks like the pattern we're using in existing choice code is arena_->New<ChoiceType>(choice->name(), ... to construct with the name instead of the ChoiceDeclaration. Is there a reason we shouldn't always construct with the ChoiceDeclaration?

Also, one more thing, could you add comments to each constructor briefly describing the use case for that constructor?

I try to minimalize code changes! This is the reason why I have not refector this part. But you are right, this should the next step after merging this PR.

But you are right, this should the next step after merging this PR.

I think it would be simpler to make that change first. Several of the issues I've flagged in other comments would be easier to address that way, and it keeps the API more consistent.

kmz42 · 2022-08-16T14:06:38Z

explorer/testdata/choice/generic_choice1.carbon

@@ -0,0 +1,30 @@
+// Part of the Carbon Language project, under the Apache License v2.0 with LLVM


Perhaps add a few more tests, particularly negative tests?

There's a bunch for generic class tests in testdata/generic_class. Probably a few of those could become good choice tests.

geoffromer

Thanks, this looks really helpful!

geoffromer · 2022-08-16T19:06:41Z

explorer/interpreter/value.h

+  auto type_args() const -> const BindingMap& { return bindings_->args(); }
+  auto declaration() const -> const ChoiceDeclaration& { return *declaration_; }
+
+  auto is_generic() const -> bool { return is_generic_; }


For consistency with NominalClassType, it seems like this should be called IsParameterized, and the return value should be computed from declaration_ and bindings_ rather than stored in a separate data member.

geoffromer · 2022-08-16T19:14:10Z

explorer/interpreter/value.h

+        alternatives_(std::move(alternatives)),
+        is_generic_(false) {}
+
+  ChoiceType(std::string name, std::vector<NamedValue> alternatives,


But you are right, this should the next step after merging this PR.

I think it would be simpler to make that change first. Several of the issues I've flagged in other comments would be easier to address that way, and it keeps the API more consistent.

geoffromer · 2022-08-16T19:15:37Z

explorer/interpreter/value.h

+  auto bindings() const -> const Bindings& { return *bindings_; }
+  auto type_args() const -> const BindingMap& { return bindings_->args(); }
+  auto declaration() const -> const ChoiceDeclaration& { return *declaration_; }


It looks like it's not safe to call these methods if the object was created using the first constructor.

geoffromer · 2022-08-16T22:13:59Z

explorer/interpreter/value.h


 private:
  Nonnull<const Declaration*> declaration_;
  Nonnull<const TuplePattern*> params_;
+  std::vector<NamedValue> alternatives_;


If I understand correctly, ParameterizedEntityName represents a name. In other words, in an expression like Optional(i32), it represents the Optional part. That being the case, it seems really weird for this class to have a list of alternatives as a data member. Could the code that needs the alternatives get them out of declaration instead?

geoffromer · 2022-08-16T22:15:25Z

explorer/testdata/choice/generic_choice1.carbon

@@ -0,0 +1,30 @@
+// Part of the Carbon Language project, under the Apache License v2.0 with LLVM


"generic_choice1" isn't a very descriptive file name. What is this file actually testing that isn't covered by generic_choice.carbon?

pmqtt · 2022-08-17T14:08:55Z

Hello folks, geoffromer and kmz42,
first thanks for the review. I have rename the test files and prettify the code ( reduceing the constructor count).
Second, I know that store alternatives in ParameterizedType is weird. The reason is, after the type checking process the Interpreter process need this information. The ChoiceDeclaration stores AlternativeSignatures and the ChoiceType need NamedValues. Without a futher type checking step isn't possible to get the NamedValue. Possible solutions are

Allow ChoiceDeclaration to store NamedValues
Change the interpreter that generic choices not handle by CallFunction
Renaming the ParametrizedType to....?

Another point is, if we want to create the NamedValue in the type checking process TypeCheckPattern and TypeCheckExp we need const_cast. The resulting code is really weird and ugly.

@geoffromer have you any ideas, or should we start with my solution?

…aration

pmqtt · 2022-08-19T09:25:14Z

@geoffromer I must apologize... I had thought NamedValue was derived from Value. This is not true, so it is possible to store the Choice-alternatives in the ChoiceDeclaration object.

jonmeow

To note on style˛I think there are always going to be differences on preferences for what style is used. While I'm happy to point out style guide issues, I also think it can be beneficial to avoid changing code when there's not a correctness or readability problem. Otherwise, someone else could end up sending a PR to change it back with equivalent justification.

jonmeow · 2022-08-24T16:42:33Z

explorer/interpreter/value.h

+        name_(declaration->name()),
+        alternatives_(declaration->members()),


Are these copies needed? Since you've started keeping a ChoiceDeclaration for IsParameterized, should name() and FindAlternative use it directly too?

jonmeow · 2022-08-24T16:45:14Z

explorer/interpreter/type_checker.cpp

@@ -2205,8 +2215,11 @@ auto TypeChecker::TypeCheckExp(Nonnull<Expression*> e,
    }
    case ExpressionKind::CallExpression: {
      auto& call = cast<CallExpression>(*e);
+


I think you're adding a lot of blank lines in this file around code that you're not otherwise editing. The rule of thumb is to minimize vertical whitespace (https://google.github.io/styleguide/cppguide.html#Vertical_Whitespace), and while I think it can be useful to separate code blocks, I think separating out single lines is probably a little more than is needed. Can you please do a pass reducing the whitespace back down?

jonmeow · 2022-08-24T17:08:38Z

explorer/interpreter/type_checker.cpp

+auto TypeChecker::TypeCheckChoiceDeclaration(Nonnull<ChoiceDeclaration*> choice,
+                                             const ImplScope& impl_scope)


These were commented out to indicate they were unused. They probably should not be uncommented in this PR, because uses are not added.

jonmeow · 2022-08-24T17:09:57Z

explorer/interpreter/resolve_names.cpp

      // Alternative names are never used unqualified, so we don't need to
      // add the alternatives to a scope, or introduce a new scope; we only
      // need to check for duplicates.


This comment seems to be referring to the below for loop, and probably shouldn't be separated from it, particularly with a blank line.

jonmeow · 2022-08-24T17:11:54Z

explorer/interpreter/type_checker.cpp

+          const TypeOfParameterizedEntityName& entity =
+              cast<TypeOfParameterizedEntityName>(
+                  call.function().static_type());
+
+          const ParameterizedEntityName& param_name = entity.name();


This feels like it's not necessary to change. I'm not seeing a particular style benefit to it, and you don't make use of entity. Can you please switch back so that we're not changing code when not necessary?

jonmeow · 2022-08-24T17:19:02Z

** picking up this review since geoffromer is out

jonmeow

Thanks! I'm happy to have this in.

Going on vacation

cefour added 9 commits August 8, 2022 14:14

added needed Typechecking

585765c

added file

b03e683

Merge remote-tracking branch 'origin/root' into feature-generic-choice

dcd82f1

add logs

e593f32

break

65de14e

finished generic choice

c7474ff

remove test files

bf9b63c

Merge remote-tracking branch 'origin/root' into feature-generic-choice

0625d0f

run pre-commit

466d6a2

kmz42 reviewed Aug 16, 2022

View reviewed changes

geoffromer previously requested changes Aug 16, 2022

View reviewed changes

cefour added 2 commits August 17, 2022 15:31

review work

dc0b97a

run precommit

a6d2f15

pmqtt requested a review from geoffromer August 17, 2022 14:09

cefour added 2 commits August 19, 2022 11:20

remove alternatives from ParameterizedType and add them to ChoiceDecl…

feff2fa

…aration

run precommit

34694dc

pmqtt requested review from kmz42 and geoffromer and removed request for geoffromer and kmz42 August 19, 2022 09:28

cefour added 6 commits August 19, 2022 14:39

reduce ChoiceType constructor parameter count

458813e

pre-commit run

fe0ad81

run update_checks.py

772d63d

prevent name collision

2dda541

first try to convert to bool

009e568

revert changes

f7f94ab

jonmeow reviewed Aug 24, 2022

View reviewed changes

add review changes from JonMeow

a3938b4

pmqtt requested a review from jonmeow August 24, 2022 17:54

jonmeow approved these changes Aug 24, 2022

View reviewed changes

jonmeow merged commit f957d4d into carbon-language:trunk Aug 24, 2022

chandlerc added the explorer Action items related to Carbon explorer code label Jan 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature generic choice #2042

Feature generic choice #2042

pmqtt commented Aug 16, 2022

kmz42 left a comment

kmz42 Aug 16, 2022

pmqtt Aug 16, 2022

kmz42 Aug 16, 2022

kmz42 Aug 16, 2022

pmqtt Aug 16, 2022

geoffromer Aug 16, 2022

kmz42 Aug 16, 2022

geoffromer left a comment

geoffromer Aug 16, 2022

geoffromer Aug 16, 2022

geoffromer Aug 16, 2022

geoffromer Aug 16, 2022

geoffromer Aug 16, 2022

pmqtt commented Aug 17, 2022

pmqtt commented Aug 19, 2022 •

edited

Loading

jonmeow left a comment

jonmeow Aug 24, 2022

jonmeow Aug 24, 2022

jonmeow Aug 24, 2022

jonmeow Aug 24, 2022

jonmeow Aug 24, 2022

jonmeow commented Aug 24, 2022

jonmeow left a comment

		@@ -0,0 +1,30 @@
		// Part of the Carbon Language project, under the Apache License v2.0 with LLVM

		name_(declaration->name()),
		alternatives_(declaration->members()),

		auto TypeChecker::TypeCheckChoiceDeclaration(Nonnull<ChoiceDeclaration*> choice,
		const ImplScope& impl_scope)

Feature generic choice #2042

Feature generic choice #2042

Conversation

pmqtt commented Aug 16, 2022

kmz42 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geoffromer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmqtt commented Aug 17, 2022

pmqtt commented Aug 19, 2022 • edited Loading

jonmeow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonmeow commented Aug 24, 2022

jonmeow left a comment

Choose a reason for hiding this comment

pmqtt commented Aug 19, 2022 •

edited

Loading