Revise "Control Flow Collections" and add implementation plan. #127

munificent · 2018-12-07T21:29:20Z

The main changes are:

Allow for in const collections.
Now that Set literals are on the way, incorporate them into this
proposal.
Add a more detailed specification of const collections since we can't
rely on imperative dynamic semantics for those.
Couple more clarifications and copy-edits.

I'm not entirely sold on allow for in consts. It's quite restricted, but also allows you to make giant constants if you really want to. I'm not sure if it's useful enough in practice to be worth adding. What do you think?

The main changes are: - Now that Set literals are on the way, incorporate them into this proposal. - Add a more detailed specification of const collections since we can't rely on imperative dynamic semantics for those. - Couple more clarifications and copy-edits.

working/control-flow-collections/feature-specification.md

leafpetersen · 2018-12-08T09:26:43Z

working/control-flow-collections/feature-specification.md

+    *   It is a compile-time error if the expression is not a constant
+        expression.
+
+    The expansion is the value of the expression.


With spread, presumably no longer just a single value. There's a bit of a chicken and egg here in that the two proposals interact - I think one of the two proposals should probably specify how they interact?

I would prefer to combine the features.

If so, then a spread is not an expression element, it's a spread element.

Lasse is right. Added a blurb about spreads now that those are a given. (This proposal was initially written before spreads were accepted.)

leafpetersen · 2018-12-08T09:27:28Z

working/control-flow-collections/feature-specification.md

+
+    *   The then element if the condition expression evaluates to `true`.
+
+    *   The else element if the condition is `false` and there is on.


s/is on/is one/

leafpetersen · 2018-12-08T09:29:43Z

working/control-flow-collections/feature-specification.md

+
+    The expansion is:
+
+    *   The then element if the condition expression evaluates to `true`.


May want to be clear that evaluation short circuits (as we are changing ?: to do). So constant evaluation of the then/else branch doesn't happen unless the condition evaluates to true/false (respectively).

Agree. The hack here is to say that both branches must be potentially constant expressions, and the one you evaluate must be constant (and hence evaluate to a value).

leafpetersen · 2018-12-08T09:36:53Z

working/control-flow-collections/implementation-plan.md

+### IntelliJ/Grok
+
+Update to use the latest analyzer with support for the feature. Likely no other
+changes explicitly needed. There a are a handful of usability features that


Intellij has its own parser which needs updating. Also github markdown.

Done. For the latter, I'm guessing you mean: https://github.com/dart-atom/dart, which is what drives Dart syntax highlighting on GitHub.

Also https://github.com/codemirror/CodeMirror/blob/master/mode/dart/dart.js ?

Not sure. @devoncarew?

The codemirror and atom grammars are largely regex based. My guess is that there won't be any changes required to support the control flow work.

leafpetersen · 2018-12-08T09:37:55Z

working/control-flow-collections/implementation-plan.md

+
+    Fix:
+
+    ```dart


Risky fix if before or after might have nulls.

Added a note.

lrhn · 2018-12-11T10:21:41Z

working/control-flow-collections/feature-specification.md

+    elements are allowed.
+
+*   It is a `for` element with an `in` clause. The iterator expression must be
+    a constant expression. In a constant list literal, the expression must


It must be a potentially constant expression wrt. the loop variable (and any other "potentially constant variables" in scope, say from nested loops). You probably can't see constructor parameters even if the const collection expression occurs in an initializer list of a const constructor - they won't go past the const prefix`.

Done, I think? I find the const stuff pretty confusing.

lrhn · 2018-12-11T10:22:56Z

working/control-flow-collections/feature-specification.md

-`for` elements cannot be used in const lists and maps.
+*   It is an expression element and the expression is constant.
+
+*   It is an `if` element and the condition expression is constant and both body


(Nitpick: or just one body element, in case there is no else)

lrhn · 2018-12-11T10:23:40Z

working/control-flow-collections/feature-specification.md

+
+    This restriction ensures that iterating over the iterable does not call
+    user code.
+


(If we integrate spreads into this document, I assume you can spread the same collections that you can iterate).

The spread proposal covers that, yes. I'm not planning to directly merge the two proposal docs, but I'll try to make sure this one calls out to spread where appropriate.

I can merge them if you really want, but I think it will just add to confusion now and be a lot of busywork.

lrhn · 2018-12-11T10:24:40Z

working/control-flow-collections/feature-specification.md

+    This restriction ensures that iterating over the iterable does not call
+    user code.
+
+


So, the disallowed ones are for(;;) and await for (and anything non-constant).

Yup. Noted.

lrhn · 2018-12-11T10:25:57Z

working/control-flow-collections/feature-specification.md

+*   It is an `if` element and the condition expression is constant and both body
+    elements are allowed.
+
+*   It is a `for` element with an `in` clause. The iterator expression must be


Which expression is the "iterator" expression, and which is "the expression" (below).

lrhn · 2018-12-11T10:37:19Z

working/control-flow-collections/feature-specification.md

+
+    The expansion is:
+
+    *   The then element if the condition expression evaluates to `true`.


Agree. The hack here is to say that both branches must be potentially constant expressions, and the one you evaluate must be constant (and hence evaluate to a value).

lrhn · 2018-12-11T10:37:54Z

working/control-flow-collections/feature-specification.md

+    *   It is a compile-time error if the for element is a C-style loop. We
+        only allow const `for-in` loops.
+
+    *   It is a compile-time error if the iterator expression is not a constant


"iterator expression" is confusing to me. I'd use "iterable expression" if anything.

lrhn · 2018-12-11T10:38:25Z

working/control-flow-collections/feature-specification.md

+        expression. In a constant list literal, the expression must evaluate to
+        an object created by a constant list or set literal, as in:
+
+        ```dart


Repeated from just above?

Oops, yeah. I reworked this halfway through and forgot to delete the first attempt. Fixed.

lrhn · 2018-12-11T10:40:25Z

working/control-flow-collections/feature-specification.md

+        const list = [for (i in []) i]; // Error.
+        ```
+
+    *   In the body element, the loop variable is considered a constant


Here you should be mentioning "potentially constant expressions". The loop variable is not constant (you can't write [x] because a list literal must be constant, not just potentially constant), but it is considered so in some cases.

We should clean that up before adding this. @eernstg

Tweaked things.

lrhn · 2018-12-11T10:42:28Z

working/control-flow-collections/feature-specification.md

+
+    1.  For each `object` in the iterated collection:
+
+        1.  Create a fresh constant and bind it to `object`.


(Again, not constant, but what we would say for parameters of a const constructor, we just need to generalize that.)

kmillikin · 2018-12-13T10:53:12Z

working/control-flow-collections/implementation-plan.md

+
+## Phase 1 (Foundation)
+
+### CFE


We will plan to have constant evaluation in the CFE to support constant control-flow collections. We don't have that yet, so that is a prerequisite. CFE team will have to make that a very high priority.

Our implementation will involve a 'block expression' in Kernel which is new and first introduced for this purpose. So depending on how you look at it, the back ends will need to support both a constant pool in .dill files and block expressions.

I'll work on estimates for those tasks.

Thanks, added a note.

I'm going to go ahead and land then and then send out further modifications as a separate PR.

devoncarew · 2018-12-14T23:24:38Z

working/control-flow-collections/implementation-plan.md

+
+Update the IntelliJ parser to support the new syntax.
+
+Update to use the latest analyzer with support for the feature. There a are a


There a are a ==> There are a

Fixed, thanks.

munificent requested review from lrhn and leafpetersen December 7, 2018 21:29

leafpetersen approved these changes Dec 8, 2018

View reviewed changes

lrhn reviewed Dec 11, 2018

View reviewed changes

kmillikin reviewed Dec 13, 2018

View reviewed changes

munificent mentioned this pull request Dec 13, 2018

hope for do expression #132

Open

Revise some more.

45e9bc2

I'm going to go ahead and land then and then send out further modifications as a separate PR.

munificent merged commit 598368e into master Dec 13, 2018

munificent deleted the revise-control-flow-collections branch December 13, 2018 21:06

devoncarew reviewed Dec 14, 2018

View reviewed changes


		* The then element if the condition expression evaluates to `true`.

		* The else element if the condition is `false` and there is on.


		The expansion is:

		* The then element if the condition expression evaluates to `true`.


		This restriction ensures that iterating over the iterable does not call
		user code.


		1. For each `object` in the iterated collection:

		1. Create a fresh constant and bind it to `object`.


		Update the IntelliJ parser to support the new syntax.

		Update to use the latest analyzer with support for the feature. There a are a

Revise "Control Flow Collections" and add implementation plan. #127

Revise "Control Flow Collections" and add implementation plan. #127

Conversation

munificent commented Dec 7, 2018

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

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

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

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

munificent Dec 13, 2018 • 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

munificent Dec 13, 2018 •

edited

Loading