Factor out AST node for function return types. #912

geoffromer · 2021-10-22T21:42:03Z

This enables us to stop treating the return type as a Pattern (which is really isn't), treat return types more consistently with other static types in the typechecker, and drop ReturnTypeContext.

Additional changes:

Merge TypeCheckFunDef with TypeOfFunDef.
Handle implicit conversions in return statements.
Require function type literals to have an explicit ->.
Move consistency check for omitted returns from TypeChecker to ResolveControlFlow.

This enables us to stop treating the return type as a Pattern (which is really isn't), treat return types more consistently with other static types in the typechecker, and drop ReturnTypeContext. Additional changes: - Merge TypeCheckFunDef with TypeOfFunDef. - Handle implicit conversions in `return` statements. - Require function type literals to have an explicit `->`. - Move consistency check for omitted returns from TypeChecker to ResolveControlFlow.

jonmeow

Note, it may be worth splitting off the -> requirement for fnty into its own discussion. I think that's a significant syntax change, and probably indicates we should get a leads decision for a path forward. I've bumped #191 to try to address this.

executable_semantics/ast/declaration.h

jonmeow · 2021-10-25T16:18:10Z

executable_semantics/ast/declaration.h

+  // Returns true if this represents an omitted return term.
+  auto is_omitted() const -> bool { return form_ == Form::Omitted; }
+
+  // Returns true if this represents an auto return term.
+  auto is_auto() const -> bool { return form_ == Form::Auto; }


Why expose the kind with boolean accessors? Wouldn't it be less code if the enum were exposed more directly? e.g., in TypeCheckFunctionDeclaration, you could write that as a switch instead of checking is_omitted and type_expression.has_value

It might be less code in the places that need to handle all three cases, but it would be more code in the places that need to give special treatment to one of the cases. The latter seem to be slightly more common, and they seem to be places where there's more need for brevity. For example, see resolve_control_flow.cpp lines 51 and 53, where calls to function_return.is_omitted() are part of larger expressions, and replacing them with
something like function_return.return_kind() == ReturnTerm::ReturnKind::Omitted would be fairly unwieldy.

FWIW, I'd still argue for just having a single accessor in order to make it clear that there are other cases being ignored, and also to make it clearer at call sites that there is a mutually exclusive state (i.e., that it can't have is_omitted == true and is_auto == true, and that both of those imply no expression). But I'll leave this up to you.

executable_semantics/interpreter/type_checker.cpp

executable_semantics/interpreter/resolve_control_flow.cpp

jonmeow · 2021-10-25T17:18:09Z

executable_semantics/interpreter/resolve_control_flow.cpp

+// Aggregate information about a function being analyzed.
+struct FunctionData {


It might be helpful to make it clearer this is only for return statements. This took me a few passes to understand while reading.

I'm not quite sure what you mean by "only for return statements". It's true that currently the only nontrivial usages of this type are in the Return case, but that's not a requirement of the type, it's a more-or-less accidental fact about how the type is currently used.

I think the accidental fact would be useful to note, at least until (or unless) it changes. I view this as a readability issue where reducing the amount of context that needs to be gained is helpful. But you call this accidental -- do you have other situations where you expect usage to expand when resolving control flow?

While it is a function, if Return is the only control flow that cares about functions, then I think it's easier to understand in that context. e.g., even just annotating "// Aggregate information about a function being analyzed. Currently only used for Return statement flow." would be an aid. Renaming it to ReturnContext would be clearer at usage sites that it's not used in other ways, and encourage a rename should it get used in more situations.

I think the accidental fact would be useful to note, at least until (or unless) it changes. I view this as a readability issue where reducing the amount of context that needs to be gained is helpful.

I don't understand how that would be helpful, so it seems like there's a gap in my mental model of how people read code. Can you help me understand? For example, can you describe a situation where omitting that information would make the code harder for a reader to understand, and what their thought process might be like?

But you call this accidental -- do you have other situations where you expect usage to expand when resolving control flow?

I don't have specific situations in mind, but I don't have a specific reason to rule it out either.

Partly, I see this as a matter of consistency -- this way, each parameter ofResolveControlFlow (other than statement itself) corresponds to a particular kind of enclosing scope that statement might transfer control to. I think it would be more confusing if one parameter was named and documented in those terms, but the other was named and documented in terms of what kind of statement uses it.

executable_semantics/syntax/parser.ypp

executable_semantics/interpreter/type_checker.cpp

jonmeow

LGTM, thank you for discussing this last night.

executable_semantics/interpreter/type_checker.cpp

jonmeow · 2021-10-27T15:38:53Z

executable_semantics/ast/declaration.cpp

+    case ReturnKind::Omitted:
+      return;
+    case ReturnKind::Auto:
+      out << "-> auto";


Stray thought: as static types are added, should they be part of the output for debugability?

Currently, Print on AST nodes is basically an "unparse" operation whose output is syntactically equivalent to the original input, in the sense that parsing the output of Print should give you an identical AST. I think that's a useful property, and I'm reluctant to abandon it.

I do think we'll eventually want to be able to print something more like a direct dump of the AST, and it would make sense to include type information in that.

executable_semantics/ast/declaration.h

executable_semantics/syntax/parser.ypp

jonmeow · 2021-10-27T15:46:52Z

executable_semantics/interpreter/resolve_control_flow.cpp

+// Aggregate information about a function being analyzed.
+struct FunctionData {


I think the accidental fact would be useful to note, at least until (or unless) it changes. I view this as a readability issue where reducing the amount of context that needs to be gained is helpful. But you call this accidental -- do you have other situations where you expect usage to expand when resolving control flow?

While it is a function, if Return is the only control flow that cares about functions, then I think it's easier to understand in that context. e.g., even just annotating "// Aggregate information about a function being analyzed. Currently only used for Return statement flow." would be an aid. Renaming it to ReturnContext would be clearer at usage sites that it's not used in other ways, and encourage a rename should it get used in more situations.

executable_semantics/interpreter/resolve_control_flow.cpp

jonmeow · 2021-10-27T15:53:33Z

executable_semantics/ast/declaration.h

+  // Returns true if this represents an omitted return term.
+  auto is_omitted() const -> bool { return form_ == Form::Omitted; }
+
+  // Returns true if this represents an auto return term.
+  auto is_auto() const -> bool { return form_ == Form::Auto; }


FWIW, I'd still argue for just having a single accessor in order to make it clear that there are other cases being ignored, and also to make it clearer at call sites that there is a mutually exclusive state (i.e., that it can't have is_omitted == true and is_auto == true, and that both of those imply no expression). But I'll leave this up to you.

This enables us to stop treating the return type as a Pattern (which is really isn't), treat return types more consistently with other static types in the typechecker, and drop ReturnTypeContext. Additional changes: - Merge TypeCheckFunDef with TypeOfFunDef. - Handle implicit conversions in `return` statements. - Require function type literals to have an explicit `->`. - Move consistency check for omitted returns from TypeChecker to ResolveControlFlow.

geoffromer added 2 commits October 22, 2021 21:34

Documentation fixes

be5789b

geoffromer added the explorer Action items related to Carbon explorer code label Oct 22, 2021

geoffromer requested a review from a team as a code owner October 22, 2021 21:42

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

jonmeow mentioned this pull request Oct 25, 2021

Should there be a function type? #191

Closed

jonmeow reviewed Oct 25, 2021

View reviewed changes

Respond to reviewer comments.

3d4f2ea

jonmeow approved these changes Oct 27, 2021

View reviewed changes

geoffromer added 2 commits October 28, 2021 19:12

Respond to reviewer comments.

da3d1d6

Merge branch 'trunk' into function

5f45214

geoffromer merged commit 3bdf7ce into carbon-language:trunk Nov 12, 2021

geoffromer deleted the function branch November 12, 2021 19:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Factor out AST node for function return types. #912

Factor out AST node for function return types. #912

geoffromer commented Oct 22, 2021

jonmeow left a comment

jonmeow Oct 25, 2021

geoffromer Oct 25, 2021

jonmeow Oct 27, 2021

jonmeow Oct 25, 2021

geoffromer Oct 25, 2021

jonmeow Oct 27, 2021

geoffromer Oct 28, 2021

jonmeow left a comment

jonmeow Oct 27, 2021

geoffromer Oct 28, 2021

jonmeow Oct 27, 2021

jonmeow Oct 27, 2021

		// Aggregate information about a function being analyzed.
		struct FunctionData {

Factor out AST node for function return types. #912

Factor out AST node for function return types. #912

Conversation

geoffromer commented Oct 22, 2021

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

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