Skip to content

fix(graindoc): Add all function parameters to the table, even if missing documentation #2293

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

Open
marcusroberts wants to merge 3 commits into
base: main
Choose a base branch
from
Open

fix(graindoc): Add all function parameters to the table, even if missing documentation #2293

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1ab296f
Initial implementation of handling missing documentation for parameters
May 22, 2025
abcf872
Tidy up the code
May 22, 2025
81e6843
Added tests for params
Jun 2, 2025
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
102 changes: 66 additions & 36 deletions compiler/graindoc/docblock.re
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -376,6 +376,68 @@ let for_value_description =

let (args, return_type) = types_for_function(~ident, vd);

// extract the documented parameters and put into a tuple for easy lookup
let param_attributes: list((string, Comment_attributes.t)) =
List.filter_map(
attr =>
switch (attr) {
| Comment_attributes.Param({attr_id}) =>
switch (attr_id) {
| PositionalParam(idx) => Some((string_of_int(idx), attr))
| LabeledParam(name) => Some((name, attr))
}
| _ => None
},
attributes,
);

let match_label_to_arg = arg => {
List.find_opt(((name, _)) => name == arg, param_attributes);
Copy link
Member

@spotandjake spotandjake Jun 2, 2025

Choose a reason for hiding this comment

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

I think List.assoc_opt or List.assq_opt might be what you want to use here https://ocaml.org/manual/5.3/api/List.html#1_Associationlists

};

let documented_args =
switch (args) {
| None => []
| Some(function_args) =>
List.mapi(
(index, (lab, exp)) => {
let param_id =
switch (lab) {
| Typedtree.Labeled(l)
| Default(l) => l.txt
| _ => string_of_int(index)
};

// First try to match by name, then by position
let matched =
switch (match_label_to_arg(param_id)) {
| Some(_) as result => result
| None => match_label_to_arg(string_of_int(index))
Copy link
Member

@spotandjake spotandjake Jun 2, 2025

Choose a reason for hiding this comment

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

Is the behaviour here that if you documented a param as x but the function only has position params we would map x to 0 for instance?

};

// Extract documentation from the matched attribute
let documentation =
switch (matched) {
| Some((_, Comment_attributes.Param({attr_desc}))) => attr_desc
| _ => ""
Copy link
Member

@spotandjake spotandjake Jun 2, 2025

Choose a reason for hiding this comment

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

Should we be throwing some sort of exception here?

};

// Get parameter type
let param_type =
switch (lookup_arg_by_label(param_id, args)) {
| Some((Labeled(_), typ)) => Printtyp.string_of_type_sch(typ)
Copy link
Member

@spotandjake spotandjake Jun 2, 2025

Choose a reason for hiding this comment

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

Nit: This could probably use an or pattern, instead of handling each branch with the same logic.

| Some((Default(_), {desc: TTyConstr(_, [typ], _)})) =>
Printtyp.string_of_type_sch(typ)
| Some((_, typ)) => Printtyp.string_of_type_sch(typ)
| None => ""
Copy link
Member

@spotandjake spotandjake Jun 2, 2025

Choose a reason for hiding this comment

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

Should we be throwing some sort of exception here? And do you know if there is any real case this would occur?

};

{param_id, param_type, param_msg: documentation};
},
function_args,
)
};

let (deprecations, since, history, params, returns, throws, examples) =
List.fold_left(
(
Expand Down Expand Up @@ -414,41 +476,9 @@ let for_value_description =
throws,
examples,
)
| Param({attr_id: param_id, attr_desc: param_msg}) =>
let (param_id, param_type) =
switch (param_id) {
| PositionalParam(idx) =>
switch (lookup_type_expr(~idx, args)) {
| Some((_, typ)) => (
string_of_int(idx),
Printtyp.string_of_type_sch(typ),
)
| None => raise(MissingUnlabeledParamType({idx: idx}))
Copy link
Member

@spotandjake spotandjake Jun 3, 2025

Choose a reason for hiding this comment

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

Why do we never throw these errors anymore?

  • Do we now allow labeling a positional arg with a label and the other way around? i.e:
/*
 * @param x: test
 */
 let test = (_ as x) => void
  • What happens now if you document a non existing param, or too many positional params?

I see we removed it here and decided to handle the cases but are we sure we want to from a linting and correctness standpoint?

}
| LabeledParam(name) =>
switch (lookup_arg_by_label(name, args)) {
| Some((Labeled(_), typ)) => (
name,
Printtyp.string_of_type_sch(typ),
)
// Default parameters have the type Option<a>; extract the type from the Option
| Some((Default(_), {desc: TTyConstr(_, [typ], _)})) => (
"?" ++ name,
Printtyp.string_of_type_sch(typ),
)
| _ => raise(MissingLabeledParamType({name: name}))
}
};

(
deprecations,
since,
history,
[{param_id, param_type, param_msg}, ...params],
returns,
throws,
examples,
);
| Param(_) =>
// do nothing as we precomputed them
(deprecations, since, history, params, returns, throws, examples)
| Returns({attr_desc: returns_msg}) =>
switch (returns) {
| Some(_) =>
Expand Down Expand Up @@ -502,7 +532,7 @@ let for_value_description =
deprecations: List.rev(deprecations),
since,
history: List.rev(history),
params: List.rev(params),
params: documented_args,
returns,
throws: List.rev(throws),
examples: List.rev(examples),
Expand Down
Loading