Drop support for ad-hoc `case ... of` expressions by JordanMartinez · Pull Request #4241 · purescript/purescript

JordanMartinez · 2022-02-22T19:36:05Z

Description of the change

Fixes #4012. I think the examples in the changelog entry are correct, but correct me if they're not.

This is for v0.15.x

Checklist:

Added a file to CHANGELOG.d for this PR (see CHANGELOG.d/README.md)
Added myself to CONTRIBUTORS.md (if this is my first contribution)
Linked any existing issues or proposals that this pull request should close
Updated or added relevant documentation
Added a test for the contribution (if applicable)

natefaubion · 2022-02-22T19:43:54Z

The examples are not correct.

-- This is ok
case wat of Foo a -> a

-- This is ok
case wat of
  Foo a -> a

-- This is ok
case wat of Foo a ->
              a

-- This is not ok
case wat of Foo a ->
  a

There is an offside rule in case expressions, where the case branches must be indented past the pattern (just like with let). In the last example, the a is not indented past the Foo a column.

It's the same issue as let, we require the expression to be indented past the binder.

-- This is ok
let Foo a = b

-- This is ok
let Foo a =
      b

-- This is ok
let
  Foo a =
    b

-- This is not ok
let Foo a =
  b

natefaubion · 2022-02-22T19:45:57Z

CHANGELOG.d/breaking_remove-two-ad-hoc-case-statements.md

@@ -0,0 +1,20 @@
+* Remove ad-hoc same-line single-branch case statements


It's not single line case statements. Single line case statements are ok. It's a particular form of non-single-line case statements when there's only one branch.

Also, a nit, but they are case expressions, not statements.

I've reworked the entry. How about now?

natefaubion · 2022-02-27T00:42:22Z

I'm personally OK with this, but the original ticket asked for a warning before removing. I think we should only remove it without a warning if the other contributors agree to it.

JordanMartinez · 2022-02-27T02:05:18Z

I'll update this to add a warning.

JordanMartinez · 2022-02-27T02:15:31Z

lib/purescript-cst/src/Language/PureScript/CST/Errors.hs

  | WarnDeprecatedConstraintInForeignImportSyntax
  | WarnDeprecatedKindImportSyntax
  | WarnDeprecatedKindExportSyntax
+  | WarnDeprecatedCaseOfOffsideSyntax


I had no idea what to call this error, so this is what I put for now. Let me know if you have a better idea for it.

JordanMartinez · 2022-02-27T02:15:51Z

lib/purescript-cst/src/Language/PureScript/CST/Errors.hs

  WarnDeprecatedKindExportSyntax ->
    "Kind exports are deprecated and will be removed in a future release. Omit the 'kind' keyword instead."
+  WarnDeprecatedCaseOfOffsideSyntax ->
+    "An expression that is not indented past its `case ... of` branch's binder is deprecated and will be removed in a future release. Indent the expression beyond its binder, similar to how let bindings work."


Seems verbose, but not sure how else to explain this. Again, up for feedback here.

I don't think we need to reference let. Maybe something like:

Dedented expressions in case branches are deprecated and will be removed in a future release. Indent the branch's expression past it's binder instead.

It's a tough concept to word succinctly.

Warning has been updated to use the above rendering. Thanks!

JordanMartinez · 2022-02-27T02:17:10Z

lib/purescript-cst/src/Language/PureScript/CST/Parser.y

+      {% addWarning (let (a,b) = whereRange $8 in [a, b]) WarnDeprecatedCaseOfOffsideSyntax *> pure (ExprCase () (CaseOf $1 $2 $3 (pure ($5, Unconditional $6 $8)))) }
  | 'case' sep(expr, ',') 'of' '\{' sep(binder1, ',') '\}' guardedCase
-      { ExprCase () (CaseOf $1 $2 $3 (pure ($5, $7))) }
+      {% addWarning (let (a,b) = guardedRange $7 in [a, b]) WarnDeprecatedCaseOfOffsideSyntax *> pure (ExprCase () (CaseOf $1 $2 $3 (pure ($5, $7)))) }


Do we need both a and b source tokens here? Or just a?

JordanMartinez · 2022-02-27T02:36:10Z

CI error seems related to a purs bundle issue.

natefaubion · 2022-02-27T16:11:09Z

src/Language/PureScript/Errors.hs

          CST.WarnDeprecatedForeignKindSyntax -> suggest $ "data " <> CST.printTokens (drop 3 toks)
          CST.WarnDeprecatedKindImportSyntax -> suggest $ CST.printTokens $ drop 1 toks
          CST.WarnDeprecatedKindExportSyntax -> suggest $ CST.printTokens $ drop 1 toks
+          CST.WarnDeprecatedCaseOfOffsideSyntax -> suggest $ CST.printTokens toks


I don't think this is correct. This is printing a replacement suggestion. This will end up suggesting a replacement of the first and last tokens of the expression.

Ah, ok. I just dropped this by returning Nothing.

JordanMartinez · 2022-02-28T14:29:32Z

Just wanted to note that in cc19ce8, I reverted the changes made in #4249

Drop ad-hoc same-line case statements

37cde5d

JordanMartinez added the type: breaking change label Feb 22, 2022

natefaubion reviewed Feb 22, 2022

View reviewed changes

Update changelog entry with more accurate example

74b402c

rhendric added this to the v0.15.0 milestone Feb 22, 2022

JordanMartinez mentioned this pull request Feb 22, 2022

Fix ad-hoc usage of case expression purescript/purescript-lists#202

Merged

4 tasks

Update lists to v6.1.0

cd50126

JordanMartinez changed the title ~~Drop ad-hoc same-line case statements~~ Drop ad-hoc case expressions Feb 23, 2022

JordanMartinez changed the title ~~Drop ad-hoc case expressions~~ Drop support for ad-hoc case ... of expressions Feb 23, 2022

JordanMartinez mentioned this pull request Feb 27, 2022

Support es modules #4232

Merged

5 tasks

Merge remote-tracking branch 'origin/master' into drop-2-case-cases

e9cf045

JordanMartinez added 2 commits February 26, 2022 20:08

Re-add syntax but warn on its usage; add tests

c8a8ab8

Update changelog entry

2a3fd78

JordanMartinez commented Feb 27, 2022

View reviewed changes

JordanMartinez mentioned this pull request Feb 27, 2022

Temporarily add addWarning to weeder roots to fix CI #4249

Merged

5 tasks

JordanMartinez added 2 commits February 26, 2022 21:19

Merge remote-tracking branch 'origin/master' into drop-2-case-cases

3420961

Drop 'addWarning' from weeder root

cc19ce8

natefaubion reviewed Feb 27, 2022

View reviewed changes

JordanMartinez added 4 commits February 28, 2022 08:14

Use Nate's suggested warning message

6bbb80d

Drop suggestion completely

6305a41

Update golden tests for new warning

393e00e

Merge remote-tracking branch 'origin/master' into drop-2-case-cases

9d0dcfc

natefaubion approved these changes Feb 28, 2022

View reviewed changes

Merge remote-tracking branch 'origin/master' into drop-2-case-cases

38da281

JordanMartinez mentioned this pull request Mar 1, 2022

Make first alpha release for v0.15.0 #4251

Merged

5 tasks

JordanMartinez merged commit 0177f71 into purescript:master Mar 1, 2022

JordanMartinez deleted the drop-2-case-cases branch March 1, 2022 19:45

		@@ -0,0 +1,20 @@
		* Remove ad-hoc same-line single-branch case statements

Conversation

JordanMartinez commented Feb 22, 2022

Uh oh!

natefaubion commented Feb 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

natefaubion commented Feb 27, 2022

Uh oh!

JordanMartinez commented Feb 27, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JordanMartinez commented Feb 27, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JordanMartinez commented Feb 28, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

natefaubion commented Feb 22, 2022 •

edited

Loading