diff --git a/CHANGELOG.md b/CHANGELOG.md index 311d5eee7f..958cc84628 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,7 @@ #### :nail_care: Polish +- Omit redundant braces around multi-statement switch case bodies when formatting. https://github.com/rescript-lang/rescript/pull/8677 - Avoid running `rescript-schema-ppx` and `sury-ppx` on source files without an `@schema` annotation. https://github.com/rescript-lang/rescript/pull/8662 #### :house: Internal diff --git a/compiler/syntax/Formatter.md b/compiler/syntax/Formatter.md index 79e264368b..433d8916d7 100644 --- a/compiler/syntax/Formatter.md +++ b/compiler/syntax/Formatter.md @@ -16,7 +16,6 @@ that all source line breaks must be retained. When changing the formatter: -- test both narrow and wide print widths; - cover comments and the parentheses needed to preserve parsing; - run `make test-syntax` and `make test-syntax-roundtrip`; - inspect snapshot changes for unrelated reformatting; diff --git a/compiler/syntax/src/res_printer.ml b/compiler/syntax/src/res_printer.ml index 7854dbe709..0c265fc518 100644 --- a/compiler/syntax/src/res_printer.ml +++ b/compiler/syntax/src/res_printer.ml @@ -5272,18 +5272,15 @@ and print_cases ~state (cases : Parsetree.case list) cmt_tbl = ]) and print_case ~state (case : Parsetree.case) cmt_tbl = + let is_block_rhs = Parsetree_viewer.is_block_expr case.pc_rhs in let rhs = - match case.pc_rhs.pexp_desc with - | Pexp_let _ | Pexp_letmodule _ | Pexp_letexception _ | Pexp_open _ - | Pexp_sequence _ -> - print_expression_block ~state - ~braces:(Parsetree_viewer.is_braced_expr case.pc_rhs) - case.pc_rhs cmt_tbl - | _ -> ( + if is_block_rhs then + print_expression_block ~state ~braces:false case.pc_rhs cmt_tbl + else let doc = print_expression_with_comments ~state case.pc_rhs cmt_tbl in match Parens.expr case.pc_rhs with | Parenthesized -> add_parens doc - | _ -> doc) + | _ -> doc in let guard = @@ -5299,12 +5296,14 @@ and print_case ~state (case : Parsetree.case) cmt_tbl = ]) in let should_inline_rhs = - match case.pc_rhs.pexp_desc with - | Pexp_construct ({txt = Longident.Lident ("()" | "true" | "false")}, _) - | Pexp_constant _ | Pexp_ident _ -> - true - | _ when Parsetree_viewer.is_huggable_rhs case.pc_rhs -> true - | _ -> false + if is_block_rhs then false + else + match case.pc_rhs.pexp_desc with + | Pexp_construct ({txt = Longident.Lident ("()" | "true" | "false")}, _) + | Pexp_constant _ | Pexp_ident _ -> + true + | _ when Parsetree_viewer.is_huggable_rhs case.pc_rhs -> true + | _ -> false in let should_indent_pattern = match case.pc_lhs.ppat_desc with diff --git a/packages/@rescript/runtime/Primitive_exceptions.res b/packages/@rescript/runtime/Primitive_exceptions.res index f657ebb013..d2f5e5a00a 100644 --- a/packages/@rescript/runtime/Primitive_exceptions.res +++ b/packages/@rescript/runtime/Primitive_exceptions.res @@ -86,14 +86,12 @@ let idMap = Dict.empty() let create = (str: string): string => { switch idMap->Dict.dangerouslyGetNonOption(str) { - | Some(v) => { - let id = v + 1 - idMap->Dict.set(str, id) - str ++ ("/" ++ (Obj.magic((id: int)): string)) - } - | None => { - idMap->Dict.set(str, 1) - str - } + | Some(v) => + let id = v + 1 + idMap->Dict.set(str, id) + str ++ ("/" ++ (Obj.magic((id: int)): string)) + | None => + idMap->Dict.set(str, 1) + str } } diff --git a/packages/dev-playground/src/Main.res b/packages/dev-playground/src/Main.res index 9a37f0ad1b..d7e97f67a3 100644 --- a/packages/dev-playground/src/Main.res +++ b/packages/dev-playground/src/Main.res @@ -174,18 +174,14 @@ let scheduleSourceOverlayWidthSync = editor => let updateActiveSourceLine = (editor, line) => Window.requestAnimationFrame(() => switch editor->Element.parentElement { - | Some(editorShell) => { - switch editorShell->Element.querySelector(".syntax-line-current") { - | Some(currentLine) => - currentLine->Element.classList->ClassList.remove("syntax-line-current") - | None => () - } - switch editorShell->Element.querySelector( - `.syntax-line[data-line="${line->Int.toString}"]`, - ) { - | Some(activeLine) => activeLine->Element.classList->ClassList.add("syntax-line-current") - | None => () - } + | Some(editorShell) => + switch editorShell->Element.querySelector(".syntax-line-current") { + | Some(currentLine) => currentLine->Element.classList->ClassList.remove("syntax-line-current") + | None => () + } + switch editorShell->Element.querySelector(`.syntax-line[data-line="${line->Int.toString}"]`) { + | Some(activeLine) => activeLine->Element.classList->ClassList.add("syntax-line-current") + | None => () } | None => () } @@ -341,36 +337,35 @@ let mappedJavaScriptNode = ( if end_ > start { let text = lineText->String.slice(~start, ~end=end_) switch mapping.original { - | Some(original) => { - let isSelected = switch selectedPosition { - | Some(position) => - position.line === mapping.generated.line && position.col === mapping.generated.col - | None => false - } - let className = isSelected - ? "source-map-mapped-segment source-map-mapped-segment-active" - : "source-map-mapped-segment" - let title = `${original.source}:${original.position.line->Int.toString}:${(original.position.col + 1) - ->Int.toString} — click to reveal in source` - nodes->Array.push( - { - let shouldNavigate = switch WindowSelection.get() { - | Some(selection) => selection->WindowSelection.isCollapsed - | None => true - } - if shouldNavigate { - onMappingSelect(mapping) - } - }} - > - {View.text(text)} - , - ) + | Some(original) => + let isSelected = switch selectedPosition { + | Some(position) => + position.line === mapping.generated.line && position.col === mapping.generated.col + | None => false } + let className = isSelected + ? "source-map-mapped-segment source-map-mapped-segment-active" + : "source-map-mapped-segment" + let title = `${original.source}:${original.position.line->Int.toString}:${(original.position.col + 1) + ->Int.toString} — click to reveal in source` + nodes->Array.push( + { + let shouldNavigate = switch WindowSelection.get() { + | Some(selection) => selection->WindowSelection.isCollapsed + | None => true + } + if shouldNavigate { + onMappingSelect(mapping) + } + }} + > + {View.text(text)} + , + ) | None => pushOutputText(nodes, text, onSourceMapSelect) } } @@ -397,18 +392,11 @@ let interactiveOutputNode = ( ) => { let output = selectedOutput(snapshot, activeTab) switch (snapshot, activeTab) { - | (Some({source: compiledSource, result: Ok({sourceMap: Some(sourceMap)})}), JavaScript) => { - let mappings = SourceMapNavigation.decodeForSource(sourceMap, compiledSource, currentSource) - mappings->Array.length > 0 - ? mappedJavaScriptNode( - output, - mappings, - selectedPosition, - onMappingSelect, - onSourceMapSelect, - ) - : outputNode(output, activeTab, onSourceMapSelect) - } + | (Some({source: compiledSource, result: Ok({sourceMap: Some(sourceMap)})}), JavaScript) => + let mappings = SourceMapNavigation.decodeForSource(sourceMap, compiledSource, currentSource) + mappings->Array.length > 0 + ? mappedJavaScriptNode(output, mappings, selectedPosition, onMappingSelect, onSourceMapSelect) + : outputNode(output, activeTab, onSourceMapSelect) | _ => outputNode(output, activeTab, onSourceMapSelect) } } @@ -785,12 +773,11 @@ module PaneSeparator = { } switch delta { - | Some(delta) => { - let (size, minFirst, minSecond) = metrics(orientation, rect) - PaneLayout.setOrientation(layout, orientation) - PaneLayout.nudge(layout, delta, ~size, ~minFirst, ~minSecond) - event->Event.preventDefault - } + | Some(delta) => + let (size, minFirst, minSecond) = metrics(orientation, rect) + PaneLayout.setOrientation(layout, orientation) + PaneLayout.nudge(layout, delta, ~size, ~minFirst, ~minSecond) + event->Event.preventDefault | None => () } }) @@ -888,45 +875,41 @@ module App = { | Some({source: compiledSource}) if SourceMapNavigation.isCurrentSource(compiledSource, Signal.peek(source)) => switch mapping.original { - | Some(original) => { - Signal.set(mappedSourcePosition, Some(original.position)) - Signal.set(mappedGeneratedPosition, Some(mapping.generated)) - Signal.set(activeLine, original.position.line) - Window.requestAnimationFrame(() => - switch Document.current->Document.getElementById(sourceEditorId) { - | Some(editor) => { - let offset = offsetForPosition(Signal.peek(source), original.position) - editor->TextAreaElement.setSelectionRange(offset, offset) - editor->Element.focus - Signal.set(activeLine, original.position.line) - updateActiveSourceLine(editor, original.position.line) - switch editor->Element.parentElement { - | Some(editorShell) => - switch editorShell->Element.querySelector( - `.syntax-line[data-line="${original.position.line->Int.toString}"]`, - ) { - | Some(line) => { - let editorRect = editor->Element.getBoundingClientRect - let lineRect = line->Element.getBoundingClientRect - let centeredScrollTop = - Signal.peek(editorScrollTop)->Int.toFloat +. - lineRect.top -. - editorRect.top -. - (editor->TextAreaElement.clientHeight->Int.toFloat -. - lineRect.height) /. 2.0 - let scrollTop = Math.Int.max(0, centeredScrollTop->Math.round->Float.toInt) - editor->TextAreaElement.setScrollTop(scrollTop) - Signal.set(editorScrollTop, editor->TextAreaElement.scrollTop) - } - | None => () - } - | None => () - } + | Some(original) => + Signal.set(mappedSourcePosition, Some(original.position)) + Signal.set(mappedGeneratedPosition, Some(mapping.generated)) + Signal.set(activeLine, original.position.line) + Window.requestAnimationFrame(() => + switch Document.current->Document.getElementById(sourceEditorId) { + | Some(editor) => + let offset = offsetForPosition(Signal.peek(source), original.position) + editor->TextAreaElement.setSelectionRange(offset, offset) + editor->Element.focus + Signal.set(activeLine, original.position.line) + updateActiveSourceLine(editor, original.position.line) + switch editor->Element.parentElement { + | Some(editorShell) => + switch editorShell->Element.querySelector( + `.syntax-line[data-line="${original.position.line->Int.toString}"]`, + ) { + | Some(line) => + let editorRect = editor->Element.getBoundingClientRect + let lineRect = line->Element.getBoundingClientRect + let centeredScrollTop = + Signal.peek(editorScrollTop)->Int.toFloat +. + lineRect.top -. + editorRect.top -. + (editor->TextAreaElement.clientHeight->Int.toFloat -. lineRect.height) /. 2.0 + let scrollTop = Math.Int.max(0, centeredScrollTop->Math.round->Float.toInt) + editor->TextAreaElement.setScrollTop(scrollTop) + Signal.set(editorScrollTop, editor->TextAreaElement.scrollTop) + | None => () } | None => () } - ) - } + | None => () + } + ) | None => () } | _ => clearMappedPositions() @@ -941,27 +924,25 @@ module App = { let currentSource = Event.value(event) let position = cursorPositionForOffset(currentSource, selectionStart) switch Signal.peek(compileResult) { - | Some({source: compiledSource, result: Ok({sourceMap: Some(sourceMap)})}) => { - let mappings = SourceMapNavigation.decodeForSource( - sourceMap, - compiledSource, - currentSource, - ) - switch SourceMapNavigation.generatedForOriginal( - mappings, - { - line: position.line, - col: position.col, - }, - ) { - | Some(mapping) => { - Signal.set(mappedSourcePosition, Some({line: position.line, col: position.col})) - Signal.set(mappedGeneratedPosition, Some(mapping.generated)) - Signal.set(activeTab, JavaScript) - scrollToGeneratedMapping() - } - | None => clearMappedPositions() - } + | Some({source: compiledSource, result: Ok({sourceMap: Some(sourceMap)})}) => + let mappings = SourceMapNavigation.decodeForSource( + sourceMap, + compiledSource, + currentSource, + ) + switch SourceMapNavigation.generatedForOriginal( + mappings, + { + line: position.line, + col: position.col, + }, + ) { + | Some(mapping) => + Signal.set(mappedSourcePosition, Some({line: position.line, col: position.col})) + Signal.set(mappedGeneratedPosition, Some(mapping.generated)) + Signal.set(activeTab, JavaScript) + scrollToGeneratedMapping() + | None => clearMappedPositions() } | _ => clearMappedPositions() } @@ -1183,16 +1164,14 @@ module App = { Window.requestAnimationFrame(() => if !disposed.contents { switch Document.current->Document.getElementById(sourceEditorId) { - | Some(editor) => { - syncSourceOverlayWidth(editor) - switch ResizeObserver.supported { - | Some(_) => { - let nextObserver = ResizeObserver.make(_ => syncSourceOverlayWidth(editor)) - observer := Some(nextObserver) - nextObserver->ResizeObserver.observe(editor) - } - | None => () - } + | Some(editor) => + syncSourceOverlayWidth(editor) + switch ResizeObserver.supported { + | Some(_) => + let nextObserver = ResizeObserver.make(_ => syncSourceOverlayWidth(editor)) + observer := Some(nextObserver) + nextObserver->ResizeObserver.observe(editor) + | None => () } | None => () } @@ -1217,26 +1196,23 @@ module App = { Window.requestAnimationFrame(() => if !disposed.contents { switch Document.current->Document.getElementById(paneLayout.containerId) { - | Some(container) => { - let updateOrientation = width => - PaneLayout.setOrientation(paneLayout, PaneLayout.orientationForWidth(width)) - - updateOrientation((container->Element.getBoundingClientRect).width) - switch ResizeObserver.supported { - | Some(_) => { - let nextObserver = ResizeObserver.make( - entries => - switch entries->Array.get(0) { - | Some(entry) => - updateOrientation((entry->ResizeObserverEntry.contentRect).width) - | None => () - }, - ) - observer := Some(nextObserver) - nextObserver->ResizeObserver.observe(container) - } - | None => () - } + | Some(container) => + let updateOrientation = width => + PaneLayout.setOrientation(paneLayout, PaneLayout.orientationForWidth(width)) + + updateOrientation((container->Element.getBoundingClientRect).width) + switch ResizeObserver.supported { + | Some(_) => + let nextObserver = ResizeObserver.make( + entries => + switch entries->Array.get(0) { + | Some(entry) => updateOrientation((entry->ResizeObserverEntry.contentRect).width) + | None => () + }, + ) + observer := Some(nextObserver) + nextObserver->ResizeObserver.observe(container) + | None => () } | None => () } diff --git a/packages/dev-playground/src/PaneLayout.res b/packages/dev-playground/src/PaneLayout.res index 8dd84e21a6..3e27f259f9 100644 --- a/packages/dev-playground/src/PaneLayout.res +++ b/packages/dev-playground/src/PaneLayout.res @@ -122,10 +122,9 @@ let moveDrag = (layout, ~pointerId, ~position) => let finishDrag = (layout, ~pointerId) => switch layout.drag.contents { - | Some(drag) if drag.pointerId === pointerId => { - layout.drag := None - Signal.update(layout.state, state => {...state, dragging: false}) - } + | Some(drag) if drag.pointerId === pointerId => + layout.drag := None + Signal.update(layout.state, state => {...state, dragging: false}) | Some(_) | None => () } diff --git a/packages/dev-playground/src/SourceMapNavigation.res b/packages/dev-playground/src/SourceMapNavigation.res index 5526152c54..afb9e2bfb3 100644 --- a/packages/dev-playground/src/SourceMapNavigation.res +++ b/packages/dev-playground/src/SourceMapNavigation.res @@ -97,14 +97,13 @@ let generatedForOriginal = (mappings, position) => { let closest: ref> = ref(None) mappings->Array.forEach(mapping => switch mapping.original { - | Some(original) if original.position.line === position.line => { - let nextDistance = distance(original.position, position) - switch closest.contents { - | None => closest := Some((nextDistance, mapping)) - | Some((currentDistance, _)) if nextDistance < currentDistance => - closest := Some((nextDistance, mapping)) - | Some(_) => () - } + | Some(original) if original.position.line === position.line => + let nextDistance = distance(original.position, position) + switch closest.contents { + | None => closest := Some((nextDistance, mapping)) + | Some((currentDistance, _)) if nextDistance < currentDistance => + closest := Some((nextDistance, mapping)) + | Some(_) => () } | Some(_) | None => () } diff --git a/tests/syntax_benchmarks/data/RedBlackTree.res b/tests/syntax_benchmarks/data/RedBlackTree.res index 72d84226c7..03155b332c 100644 --- a/tests/syntax_benchmarks/data/RedBlackTree.res +++ b/tests/syntax_benchmarks/data/RedBlackTree.res @@ -194,25 +194,24 @@ let rotateRight = (rbt, node) => { let rec findInsert = (rbt, node, nodeToInsert, value) => { switch node { | None => None - | Some(node) => { - let cmp = rbt.compare(value, node.value) - if cmp === 0 { - Some(node) - } else if cmp < 0 { - if node.left !== None { - rbt->findInsert(node.left, nodeToInsert, value) - } else { - nodeToInsert.parent = Some(node) - node.left = Some(nodeToInsert) - None - } - } else if node.right !== None { - rbt->findInsert(node.right, nodeToInsert, value) + | Some(node) => + let cmp = rbt.compare(value, node.value) + if cmp === 0 { + Some(node) + } else if cmp < 0 { + if node.left !== None { + rbt->findInsert(node.left, nodeToInsert, value) } else { nodeToInsert.parent = Some(node) - node.right = Some(nodeToInsert) + node.left = Some(nodeToInsert) None } + } else if node.right !== None { + rbt->findInsert(node.right, nodeToInsert, value) + } else { + nodeToInsert.parent = Some(node) + node.right = Some(nodeToInsert) + None } } } diff --git a/tests/syntax_tests/data/printer/expr/expected/switch.res.txt b/tests/syntax_tests/data/printer/expr/expected/switch.res.txt index d5410b1873..cd562a9405 100644 --- a/tests/syntax_tests/data/printer/expr/expected/switch.res.txt +++ b/tests/syntax_tests/data/printer/expr/expected/switch.res.txt @@ -35,10 +35,9 @@ switch count { // Expression in braces | 5 => expr // Block -| 6 => { - let _ = 123 - Console.log("Must be block") - } +| 6 => + let _ = 123 + Console.log("Must be block") } switch route { @@ -55,9 +54,21 @@ switch route { } switch x { -| A => { - let _ = 1 - let _ = 2 - } // no blank line below +| A => + let _ = 1 + let _ = 2 // no blank line below | B => () } + +switch value { +| A => + // Keep this comment with the first statement. + let local = 1 + local +| B => + let local = 2 + local +| C => + foo() + {bar()} +} diff --git a/tests/syntax_tests/data/printer/expr/switch.res b/tests/syntax_tests/data/printer/expr/switch.res index 0313cab633..5fd9aa470d 100644 --- a/tests/syntax_tests/data/printer/expr/switch.res +++ b/tests/syntax_tests/data/printer/expr/switch.res @@ -57,3 +57,18 @@ switch x { } // no blank line below | B => () } + +switch value { +| A => { + // Keep this comment with the first statement. + let local = 1 + local + } +| B => + let local = 2 + local +| C => { + foo() + {bar()} + } +} diff --git a/tests/tests/src/UntaggedVariants.res b/tests/tests/src/UntaggedVariants.res index 051bd70cd0..9b2ea55587 100644 --- a/tests/tests/src/UntaggedVariants.res +++ b/tests/tests/src/UntaggedVariants.res @@ -85,10 +85,9 @@ module Unknown = { switch x { | A => "a" | B => "b" - | Unknown(v) => { - Console.log(x) - "Unknown" - } + | Unknown(v) => + Console.log(x) + "Unknown" } } @@ -500,15 +499,13 @@ module MergeCases = { // The actions are alpha-equivalent, not merely identical constants. let shareAlphaEquivalentStringActions = value => switch value { - | 0 => { - let result = sideEffect() - (result, "shared") - } + | 0 => + let result = sideEffect() + (result, "shared") | 1 => (sideEffect(), "different") - | 2 => { - let result = sideEffect() - (result, "shared") - } + | 2 => + let result = sideEffect() + (result, "shared") | _ => (sideEffect(), "fallback") } diff --git a/tests/tests/src/stdlib/intl/Stdlib_IntlTests.res b/tests/tests/src/stdlib/intl/Stdlib_IntlTests.res index 0031ba091a..2f208730ec 100644 --- a/tests/tests/src/stdlib/intl/Stdlib_IntlTests.res +++ b/tests/tests/src/stdlib/intl/Stdlib_IntlTests.res @@ -38,10 +38,9 @@ try { | JsExn(e) => switch JsExn.message(e)->Option.map(String.toLowerCase) { | Some("invalid key : someinvalidkey") => Console.log("Caught expected error") - | message => { - Console.warn(`Unexpected error message: "${message->Option.getUnsafe}"`) - JsExn.throw(e) - } + | message => + Console.warn(`Unexpected error message: "${message->Option.getUnsafe}"`) + JsExn.throw(e) } | e => switch JsExn.fromException(e) {