Make the AST mutable #849

jonmeow · 2021-09-23T00:08:35Z

The code is pretty intertwined: having the AST be truly mutable means (to me) changing parser.ypp to return non-const values, but then the way things are passed around between objects should be non-const (particularly an issue with lists), which then creates issues with construction of lists in the TypeChecker, which then TypeChecker needs to mostly be non-const.

Due to the difficulties in breaking this apart, whereas I'd previously considering refactoring accessor naming in the same PR, I've largely avoided doing so. The intent is then that this PR focuses mainly on const -> non-const AST behavior.

call_main moves out of interpreter.cpp so that interpreter.cpp can receive a fully const AST.

geoffromer · 2021-09-24T20:05:29Z

executable_semantics/ast/declaration.h

@@ -108,22 +110,21 @@ class ChoiceDeclaration : public Declaration {
  }

  auto Name() const -> const std::string& { return name; }
-  auto Alternatives() const -> const
-      std::vector<std::pair<std::string, Nonnull<const Expression*>>>& {
+  auto Alternatives() const


This is a const method, but it's giving non-const access to the expressions, which I don't think is right. To fix that, I think we'll need to replace std::pair with a proper class with its own const/non-const accessor pair (which IMO we should do anyway). Assuming you don't want to do that in this PR, could we add a TODO, and switch alternatives back to std::vector<std::pair<std::string, Nonnull<const Expression*>>> in the meantime?

Doing this in #853

geoffromer · 2021-09-24T20:14:08Z

executable_semantics/ast/function_definition.h

@@ -43,10 +42,10 @@ struct FunctionDefinition {
  SourceLocation source_location;
  std::string name;
  std::vector<GenericBinding> deduced_parameters;
-  Nonnull<const TuplePattern*> param_pattern;
-  Nonnull<const Pattern*> return_type;
+  Nonnull<TuplePattern*> param_pattern;


This will allow code holding a const FunctionDefinition* to mutate that definition's parameters. To fix that, I think we'll need to either turn this struct into a class with const/non-const accessors, or wrap the pointer members in something like propagate_const. And unfortunately, I think the same goes for pretty much every pointer member of every struct that this PR touches (but it looks like maybe there aren't too many of those?).

If you don't want to do that in this PR, could we add the const back to those members, and flag it with a TODO?

Switching in #852, that'll block this PR

geoffromer · 2021-09-24T20:24:08Z

executable_semantics/interpreter/type_checker.cpp

@@ -248,7 +248,7 @@ auto TypeChecker::Substitute(TypeEnv dict, Nonnull<const Value*> type)
  }
 }

-auto TypeChecker::TypeCheckExp(Nonnull<const Expression*> e, TypeEnv types,
+auto TypeChecker::TypeCheckExp(Nonnull<Expression*> e, TypeEnv types,


What's the rationale for dropping the const here? AFAICT this function doesn't actually need mutable access unless/until we update it to modify the AST in place rather than producing a copy.

Trying to explain on #executable_semantics

To recap for future reference, the key issue is that in some circumstances, TypeCheckExp will reuse the input AST node as part of the output. That means in order for the output to be mutable, the input needs to be mutable. I think it's an open question whether that's the right approach long-term, but I think for purposes of this PR, keeping it non-const makes sense.

executable_semantics/interpreter/type_checker.cpp

jonmeow

Trying to break out related PRs rather than doing class refactorings here, discussing in #executable_semantics

jonmeow · 2021-09-24T20:54:14Z

executable_semantics/interpreter/type_checker.cpp

@@ -248,7 +248,7 @@ auto TypeChecker::Substitute(TypeEnv dict, Nonnull<const Value*> type)
  }
 }

-auto TypeChecker::TypeCheckExp(Nonnull<const Expression*> e, TypeEnv types,
+auto TypeChecker::TypeCheckExp(Nonnull<Expression*> e, TypeEnv types,


Trying to explain on #executable_semantics

executable_semantics/interpreter/type_checker.cpp

jonmeow · 2021-09-24T22:14:38Z

executable_semantics/ast/function_definition.h

@@ -43,10 +42,10 @@ struct FunctionDefinition {
  SourceLocation source_location;
  std::string name;
  std::vector<GenericBinding> deduced_parameters;
-  Nonnull<const TuplePattern*> param_pattern;
-  Nonnull<const Pattern*> return_type;
+  Nonnull<TuplePattern*> param_pattern;


Switching in #852, that'll block this PR

jonmeow · 2021-09-24T22:59:21Z

executable_semantics/ast/declaration.h

@@ -108,22 +110,21 @@ class ChoiceDeclaration : public Declaration {
  }

  auto Name() const -> const std::string& { return name; }
-  auto Alternatives() const -> const
-      std::vector<std::pair<std::string, Nonnull<const Expression*>>>& {
+  auto Alternatives() const


Doing this in #853

Splitting out the task from #849

geoffromer · 2021-09-27T16:20:00Z

executable_semantics/interpreter/type_checker.cpp

@@ -248,7 +248,7 @@ auto TypeChecker::Substitute(TypeEnv dict, Nonnull<const Value*> type)
  }
 }

-auto TypeChecker::TypeCheckExp(Nonnull<const Expression*> e, TypeEnv types,
+auto TypeChecker::TypeCheckExp(Nonnull<Expression*> e, TypeEnv types,


To recap for future reference, the key issue is that in some circumstances, TypeCheckExp will reuse the input AST node as part of the output. That means in order for the output to be mutable, the input needs to be mutable. I think it's an open question whether that's the right approach long-term, but I think for purposes of this PR, keeping it non-const makes sense.

Splitting out the task from #849

The code is pretty intertwined: having the AST be truly mutable means (to me) changing parser.ypp to return non-const values, but then the way things are passed around between objects should be non-const (particularly an issue with lists), which then creates issues with construction of lists in the TypeChecker, which then TypeChecker needs to mostly be non-const. Due to the difficulties in breaking this apart, whereas I'd previously considering refactoring accessor naming in the same PR, I've largely avoided doing so. The intent is then that this PR focuses mainly on const -> non-const AST behavior. call_main moves out of interpreter.cpp so that interpreter.cpp can receive a fully const AST.

jonmeow added 3 commits September 22, 2021 20:31

Checkpoint

10436e0

Merge branch 'trunk' into mutable-ast

58bca2a

Building

b4b8893

jonmeow requested a review from geoffromer September 23, 2021 00:08

jonmeow requested a review from a team as a code owner September 23, 2021 00:08

google-cla bot added the cla: yes PR meets CLA requirements according to bot. label Sep 23, 2021

Merge branch 'trunk' into mutable-ast

b19cd6a

geoffromer reviewed Sep 24, 2021

View reviewed changes

jonmeow mentioned this pull request Sep 24, 2021

Switch FunctionDefinition to a class #852

Merged

jonmeow commented Sep 24, 2021

View reviewed changes

jonmeow added a commit that referenced this pull request Sep 27, 2021

Switch FunctionDefinition to a class (#852)

fa119ac

Splitting out the task from #849

geoffromer approved these changes Sep 27, 2021

View reviewed changes

merge

72caa47

jonmeow merged commit 904c6f7 into carbon-language:trunk Sep 27, 2021

jonmeow deleted the mutable-ast branch September 27, 2021 17:57

chandlerc pushed a commit that referenced this pull request Jun 28, 2022

Switch FunctionDefinition to a class (#852)

6ab2bff

Splitting out the task from #849

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make the AST mutable #849

Make the AST mutable #849

jonmeow commented Sep 23, 2021 •

edited

Loading

geoffromer Sep 24, 2021

jonmeow Sep 24, 2021

geoffromer Sep 24, 2021

jonmeow Sep 24, 2021

geoffromer Sep 24, 2021

jonmeow Sep 24, 2021

geoffromer Sep 27, 2021

jonmeow left a comment

jonmeow Sep 24, 2021

jonmeow Sep 24, 2021

jonmeow Sep 24, 2021

geoffromer Sep 27, 2021

Make the AST mutable #849

Make the AST mutable #849

Conversation

jonmeow commented Sep 23, 2021 • edited Loading

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

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

jonmeow commented Sep 23, 2021 •

edited

Loading