Skip to content

Improve error message for duplicate labels #6415

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Sep 27, 2023
Merged

Improve error message for duplicate labels #6415

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions jscomp/build_tests/super_errors/expected/duplicate_labels_error.res.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@

We've found a bug for you!
/.../fixtures/duplicate_labels_error.res:3:3-6

1 │ type rcrd = {
2 │ name: string,
3 │ name: int,
4 │ }
5 │

The field name is defined several times in the record rcrd. Fields can only be added once to a record.
2 changes: 1 addition & 1 deletion jscomp/build_tests/super_errors/expected/record_type_spreads.res.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,4 @@
5 │ type t3 = {...t, ...t2}
6 │

Two labels are named x
The field x is defined several times in the record t3. Fields can only be added once to a record.
4 changes: 4 additions & 0 deletions jscomp/build_tests/super_errors/fixtures/duplicate_labels_error.res
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
type rcrd = {
name: string,
name: int,
}
18 changes: 10 additions & 8 deletions jscomp/ml/typedecl.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ type native_repr_kind = Unboxed | Untagged
type error =
Repeated_parameter
| Duplicate_constructor of string
| Duplicate_label of string
| Duplicate_label of string * string option
| Recursive_abbrev of string
| Cycle_in_def of string * type_expr
| Definition_mismatch of type_expr * Includecore.type_mismatch list
Expand Down Expand Up @@ -207,17 +207,17 @@ let make_params env params =
in
List.map make_param params

let transl_labels env closed lbls =
let transl_labels ?recordName env closed lbls =
if !Config.bs_only then
match !Builtin_attributes.check_duplicated_labels lbls with
| None -> ()
| Some {loc;txt=name} -> raise (Error(loc,Duplicate_label name))
| Some {loc;txt=name} -> raise (Error(loc,Duplicate_label (name, recordName)))
else (
let all_labels = ref StringSet.empty in
List.iter
(fun {pld_name = {txt=name; loc}} ->
if StringSet.mem name !all_labels then
raise(Error(loc, Duplicate_label name));
raise(Error(loc, Duplicate_label (name, recordName)));
all_labels := StringSet.add name !all_labels)
lbls);
let mk {pld_name=name;pld_mutable=mut;pld_type=arg;pld_loc=loc;
Expand Down Expand Up @@ -501,7 +501,7 @@ let transl_declaration ~typeRecordAsObject env sdecl id =
{typ with ptyp_desc = Ptyp_constr ({txt = Lident "option"; loc=typ.ptyp_loc}, [typ])}
else typ in
{lbl with pld_type = typ }) in
let lbls, lbls' = transl_labels env true lbls in
let lbls, lbls' = transl_labels ~recordName:(sdecl.ptype_name.txt) env true lbls in
let lbls_opt = match Record_type_spread.has_type_spread lbls with
| true ->
let rec extract t = match t.desc with
Expand Down Expand Up @@ -545,7 +545,7 @@ let transl_declaration ~typeRecordAsObject env sdecl id =
| [] -> ()
| lbl::rest ->
let name = lbl.ld_id.name in
if StringSet.mem name seen then raise(Error(loc, Duplicate_label name));
if StringSet.mem name seen then raise(Error(loc, Duplicate_label (name, Some sdecl.ptype_name.txt)));
check_duplicates loc rest (StringSet.add name seen) in
(match lbls_opt with
| Some (lbls, lbls') ->
Expand Down Expand Up @@ -1998,8 +1998,10 @@ let report_error ppf = function
fprintf ppf "A type parameter occurs several times"
| Duplicate_constructor s ->
fprintf ppf "Two constructors are named %s" s
| Duplicate_label s ->
fprintf ppf "Two labels are named %s" s
| Duplicate_label (s, None) ->
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious: when does this case fire?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inline records, I believe.

fprintf ppf "The field @{<info>%s@} is defined several times in this record. Fields can only be added once to a record." s
| Duplicate_label (s, Some recordName) ->
fprintf ppf "The field @{<info>%s@} is defined several times in the record @{<info>%s@}. Fields can only be added once to a record." s recordName
Comment on lines +2001 to +2004
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose the error message might be used for objects as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Objects are a bit trickier, and needs a general overhaul. Doesn't seem like overwriting object fields is an error at all, although you can't change their type of course.

| Recursive_abbrev s ->
fprintf ppf "The type abbreviation %s is cyclic" s
| Cycle_in_def (s, ty) ->
Expand Down