Merge pull request #7206 from roc-lang/record-suffix-warn-tweaks

Improve record field suffix warnings

crates/cli/tests/cli_tests.rs

@@ -955,6 +955,29 @@ mod cli_tests {
 
             cli_build.run().assert_clean_stdout(expected_out);
         }
+
+        #[test]
+        #[cfg_attr(windows, ignore)]
+        fn effectful_suffixed_record_field() {
+            build_platform_host();
+
+            let cli_build = ExecCli::new(
+                roc_cli::CMD_DEV,
+                file_from_root(
+                    "crates/cli/tests/test-projects/effectful",
+                    "suffixed_record_field.roc",
+                ),
+            );
+
+            let expected_output = "notEffectful: hardcoded\neffectful: from stdin\n";
+
+            cli_build.check_build_and_run(
+                expected_output,
+                ALLOW_VALGRIND,
+                Some("from stdin"),
+                None,
+            );
+        }
     }
 
     // this is for testing the benchmarks (on small inputs), to perform proper benchmarks see crates/cli/benches/README.md

crates/cli/tests/test-projects/effectful/suffixed_record_field.roc

@@ -0,0 +1,22 @@
+app [main!] { pf: platform "../test-platform-effects-zig/main.roc" }
+
+import pf.Effect
+
+Fx : {
+    getLine!: {} => Str,
+}
+
+main! : {} => {}
+main! = \{} ->
+    notEffectful : Fx
+    notEffectful = {
+        getLine!: \{} -> "hardcoded"
+    }
+
+    effectful : Fx
+    effectful = {
+        getLine!: Effect.getLine!
+    }
+
+    Effect.putLine! "notEffectful: $(notEffectful.getLine! {})"
+    Effect.putLine! "effectful: $(effectful.getLine! {})"

crates/compiler/can/src/annotation.rs

@@ -2,12 +2,12 @@ use crate::env::Env;
 use crate::procedure::{QualifiedReference, References};
 use crate::scope::{PendingAbilitiesInScope, Scope, SymbolLookup};
 use roc_collections::{ImMap, MutSet, SendMap, VecMap, VecSet};
-use roc_module::ident::{Ident, Lowercase, TagName};
+use roc_module::ident::{Ident, IdentSuffix, Lowercase, TagName};
 use roc_module::symbol::Symbol;
 use roc_parse::ast::{
     AssignedField, ExtractSpaces, FunctionArrow, Pattern, Tag, TypeAnnotation, TypeHeader,
 };
-use roc_problem::can::ShadowKind;
+use roc_problem::can::{Problem, ShadowKind};
 use roc_region::all::{Loc, Region};
 use roc_types::subs::{VarStore, Variable};
 use roc_types::types::{
@@ -1378,6 +1378,8 @@ fn can_assigned_fields<'a>(
                     );
 
                     let label = Lowercase::from(field_name.value);
+                    check_record_field_suffix(env, label.suffix(), &field_type, &loc_field.region);
+
                     field_types.insert(label.clone(), RigidRequired(field_type));
 
                     break 'inner label;
@@ -1396,6 +1398,8 @@ fn can_assigned_fields<'a>(
                     );
 
                     let label = Lowercase::from(field_name.value);
+                    check_record_field_suffix(env, label.suffix(), &field_type, &loc_field.region);
+
                     field_types.insert(label.clone(), RigidOptional(field_type));
 
                     break 'inner label;
@@ -1450,6 +1454,23 @@ fn can_assigned_fields<'a>(
     field_types
 }
 
+fn check_record_field_suffix(
+    env: &mut Env,
+    suffix: IdentSuffix,
+    field_type: &Type,
+    region: &Region,
+) {
+    match (suffix, field_type) {
+        (IdentSuffix::None, Type::Function(_, _, _, fx)) if **fx == Type::Effectful => env
+            .problems
+            .push(Problem::UnsuffixedEffectfulRecordField(*region)),
+        (IdentSuffix::Bang, Type::Function(_, _, _, fx)) if **fx == Type::Pure => {
+            env.problems.push(Problem::SuffixedPureRecordField(*region))
+        }
+        _ => {}
+    }
+}
+
 // TODO trim down these arguments!
 #[allow(clippy::too_many_arguments)]
 fn can_assigned_tuple_elems(

crates/compiler/can/src/constraint.rs

@@ -618,15 +618,10 @@ impl Constraints {
         Constraint::FxSuffix(constraint_index)
     }
 
-    pub fn fx_record_field_suffix(
-        &mut self,
-        suffix: IdentSuffix,
-        variable: Variable,
-        region: Region,
-    ) -> Constraint {
+    pub fn fx_record_field_unsuffixed(&mut self, variable: Variable, region: Region) -> Constraint {
         let type_index = Self::push_type_variable(variable);
         let constraint = FxSuffixConstraint {
-            kind: FxSuffixKind::RecordField(suffix),
+            kind: FxSuffixKind::UnsuffixedRecordField,
             type_index,
             region,
         };
@@ -952,14 +947,14 @@ pub struct FxSuffixConstraint {
 pub enum FxSuffixKind {
     Let(Symbol),
     Pattern(Symbol),
-    RecordField(IdentSuffix),
+    UnsuffixedRecordField,
 }
 
 impl FxSuffixKind {
     pub fn suffix(&self) -> IdentSuffix {
         match self {
             Self::Let(symbol) | Self::Pattern(symbol) => symbol.suffix(),
-            Self::RecordField(suffix) => *suffix,
+            Self::UnsuffixedRecordField => IdentSuffix::None,
         }
     }
 }

crates/compiler/constrain/src/expr.rs

@@ -24,7 +24,7 @@ use roc_can::pattern::Pattern;
 use roc_can::traverse::symbols_introduced_from_pattern;
 use roc_collections::all::{HumanIndex, MutMap, SendMap};
 use roc_collections::VecMap;
-use roc_module::ident::Lowercase;
+use roc_module::ident::{IdentSuffix, Lowercase};
 use roc_module::symbol::{ModuleId, Symbol};
 use roc_region::all::{Loc, Region};
 use roc_types::subs::{IllegalCycleMark, Variable};
@@ -284,9 +284,14 @@ pub fn constrain_expr(
                     let (field_type, field_con) =
                         constrain_field(types, constraints, env, field_var, loc_field_expr);
 
-                    let check_field_con =
-                        constraints.fx_record_field_suffix(label.suffix(), field_var, field.region);
-                    let field_con = constraints.and_constraint([field_con, check_field_con]);
+                    let field_con = match label.suffix() {
+                        IdentSuffix::None => {
+                            let check_field_con =
+                                constraints.fx_record_field_unsuffixed(field_var, field.region);
+                            constraints.and_constraint([field_con, check_field_con])
+                        }
+                        IdentSuffix::Bang => field_con,
+                    };
 
                     field_vars.push(field_var);
                     field_types.insert(label.clone(), RecordField::Required(field_type));

crates/compiler/load/tests/test_reporting.rs

@@ -101,7 +101,7 @@ mod test_reporting {
         use std::fs::File;
         use std::io::Write;
 
-        let module_src = if src.starts_with("app") {
+        let module_src = if src.starts_with("app") || src.starts_with("module") {
             maybe_save_parse_test_case(subdir, src, false);
             // this is already a module
             src.to_string()
@@ -15007,33 +15007,57 @@ All branches in an `if` must have the same type!
     );
 
     test_report!(
-        suffixed_pure_in_record,
+        unsuffixed_fx_in_record_annotation,
         indoc!(
             r#"
-            app [main!] { pf: platform "../../../../../crates/cli/tests/test-projects/test-platform-effects-zig/main.roc" }
+            module [Fx]
 
-            import pf.Effect
-
-            main! = \{} ->
-                notFx = {
-                    trim!: Str.trim
-                }
-                Effect.putLine! (notFx.trim! " hello ")
+            Fx : {
+                getLine: {} => Str
+            }
             "#
         ),
-        @r###"
+        @r"
+    ── MISSING EXCLAMATION in /code/proj/Main.roc ──────────────────────────────────
+
+    The type of this record field is an effectful function, but its name
+    does not indicate so:
+
+    4│      getLine: {} => Str
+            ^^^^^^^^^^^^^^^^^^
+
+    Add an exclamation mark at the end, like:
+
+        { readFile!: Str => Str }
+
+    This will help readers identify it as a source of effects.
+    "
+    );
+
+    test_report!(
+        suffixed_pure_fn_in_record_annotation,
+        indoc!(
+            r#"
+            module [Fx]
+
+            Fx : {
+                getLine!: {} -> Str
+            }
+            "#
+        ),
+        @r"
     ── UNNECESSARY EXCLAMATION in /code/proj/Main.roc ──────────────────────────────
 
-    This field's value is a pure function, but its name suggests
-    otherwise:
+    The type of this record field is a pure function, but its name
+    suggests otherwise:
 
-    7│          trim!: Str.trim
-                ^^^^^^^^^^^^^^^
+    4│      getLine!: {} -> Str
+            ^^^^^^^^^^^^^^^^^^^
 
     The exclamation mark at the end is reserved for effectful functions.
 
-    Hint: Did you forget to run an effect? Is the type annotation wrong?
-    "###
+    Hint: Did you mean to use `=>` instead of `->`?
+    "
     );
 
     test_report!(

crates/compiler/problem/src/can.rs

@@ -252,6 +252,8 @@ pub enum Problem {
         region: Region,
     },
     StmtAfterExpr(Region),
+    UnsuffixedEffectfulRecordField(Region),
+    SuffixedPureRecordField(Region),
 }
 
 #[derive(Debug, Clone, Copy, PartialEq, Eq)]
@@ -337,6 +339,9 @@ impl Problem {
             Problem::StatementsAfterReturn { .. } => Warning,
             Problem::ReturnAtEndOfFunction { .. } => Warning,
             Problem::StmtAfterExpr(_) => Fatal,
+            Problem::UnsuffixedEffectfulRecordField(_) | Problem::SuffixedPureRecordField(..) => {
+                Warning
+            }
         }
     }
 
@@ -502,11 +507,14 @@ impl Problem {
             | Problem::DefsOnlyUsedInRecursion(_, region)
             | Problem::ReturnOutsideOfFunction { region }
             | Problem::StatementsAfterReturn { region }
-            | Problem::ReturnAtEndOfFunction { region } => Some(*region),
+            | Problem::ReturnAtEndOfFunction { region }
+            | Problem::UnsuffixedEffectfulRecordField(region)
+            | Problem::SuffixedPureRecordField(region) => Some(*region),
             Problem::RuntimeError(RuntimeError::CircularDef(cycle_entries))
             | Problem::BadRecursion(cycle_entries) => {
                 cycle_entries.first().map(|entry| entry.expr_region)
             }
+
             Problem::StmtAfterExpr(region) => Some(*region),
             Problem::RuntimeError(RuntimeError::UnresolvedTypeVar)
             | Problem::RuntimeError(RuntimeError::ErroneousType)

crates/reporting/src/error/canonicalize.rs

@@ -65,6 +65,8 @@ const DUPLICATE_IMPLEMENTATION: &str = "DUPLICATE IMPLEMENTATION";
 const UNNECESSARY_IMPLEMENTATIONS: &str = "UNNECESSARY IMPLEMENTATIONS";
 const INCOMPLETE_ABILITY_IMPLEMENTATION: &str = "INCOMPLETE ABILITY IMPLEMENTATION";
 const STATEMENT_AFTER_EXPRESSION: &str = "STATEMENT AFTER EXPRESSION";
+const MISSING_EXCLAMATION: &str = "MISSING EXCLAMATION";
+const UNNECESSARY_EXCLAMATION: &str = "UNNECESSARY EXCLAMATION";
 
 pub fn can_problem<'b>(
     alloc: &'b RocDocAllocator<'b>,
@@ -1427,6 +1429,42 @@ pub fn can_problem<'b>(
 
             title = STATEMENT_AFTER_EXPRESSION.to_string();
         }
+
+        Problem::UnsuffixedEffectfulRecordField(region) => {
+            doc = alloc.stack([
+                alloc.reflow(
+                    "The type of this record field is an effectful function, but its name does not indicate so:",
+                ),
+                alloc.region(lines.convert_region(region), severity),
+                alloc.reflow("Add an exclamation mark at the end, like:"),
+                alloc
+                    .parser_suggestion("{ readFile!: Str => Str }")
+                    .indent(4),
+                alloc.reflow("This will help readers identify it as a source of effects."),
+            ]);
+
+            title = MISSING_EXCLAMATION.to_string();
+        }
+
+        Problem::SuffixedPureRecordField(region) => {
+            doc = alloc.stack([
+                alloc.reflow(
+                    "The type of this record field is a pure function, but its name suggests otherwise:",
+                ),
+                alloc.region(lines.convert_region(region), severity),
+                alloc
+                    .reflow("The exclamation mark at the end is reserved for effectful functions."),
+                alloc.concat([
+                    alloc.hint("Did you mean to use "),
+                    alloc.keyword("=>"),
+                    alloc.text(" instead of "),
+                    alloc.keyword("->"),
+                    alloc.text("?"),
+                ]),
+            ]);
+
+            title = UNNECESSARY_EXCLAMATION.to_string();
+        }
     };
 
     Report {

crates/reporting/src/error/type.rs

@@ -409,7 +409,7 @@ pub fn type_problem<'b>(
                     FxSuffixKind::Pattern(_) => alloc.reflow(
                         "This is an effectful function, but its name does not indicate so:",
                     ),
-                    FxSuffixKind::RecordField(_) => {
+                    FxSuffixKind::UnsuffixedRecordField => {
                         unreachable!()
                     }
                 },
@@ -427,7 +427,7 @@ pub fn type_problem<'b>(
                 severity,
             })
         }
-        UnsuffixedEffectfulFunction(region, FxSuffixKind::RecordField(_)) => {
+        UnsuffixedEffectfulFunction(region, FxSuffixKind::UnsuffixedRecordField) => {
             let stack = [
                 alloc.reflow(
                     "This field's value is an effectful function, but its name does not indicate so:",
@@ -455,9 +455,9 @@ pub fn type_problem<'b>(
                     FxSuffixKind::Pattern(_) => {
                         alloc.reflow("This is a pure function, but its name suggests otherwise:")
                     }
-                    FxSuffixKind::RecordField(_) => alloc.reflow(
-                        "This field's value is a pure function, but its name suggests otherwise:",
-                    ),
+                    FxSuffixKind::UnsuffixedRecordField => {
+                        unreachable!()
+                    }
                 },
                 alloc.region(lines.convert_region(region), severity),
                 alloc