Allow to omit parent type in constructor reference. #933

panglesd · 2023-03-02T15:14:58Z

Currently, it is not possible to refer to constructors without specifying the parent type, except if they are in scope.

This PR fixes this by extending parents of constructor references to signatures.

The first commit adds the test. The second fixes the issue similarly to what it is done with field (which already allows omitting the type parent). The last two commits are refactoring that improve the guarantee given by the subtyping.

(Made with @EmileTrotignon)

Julow

Looks good :)

Julow · 2023-03-03T09:25:48Z

src/xref2/ref_tools.ml

+    (* Let's pretend we didn't see the field and say we didn't find anything. *)
+    Error (`Find_by_name (`Cons, name))
+
+  let in_parent _env (parent : label_parent_lookup_result) name =


Perhaps we need a _lookup_result that correspond to Reference.Parent to remove the impossible cases ?

Julow · 2023-03-03T09:30:15Z

test/xref2/github_issue_447.t/a.mli

+  type t = Foo
+end
+
+(** {!M.constructor-Foo}


It could be worth checking for M.Foo and M.t.Foo, which works fine in my testing.

dbuenzli · 2023-03-30T06:43:25Z

@panglesd since you are in the area do you maybe see a fix for #932 at the same time ?

panglesd · 2023-03-30T07:04:22Z

The new commits consist of:

renaming types, since parent wasn't explicit enough
implementing @Julow's comment Allow to omit parent type in constructor reference. #933 (comment)

About #933 (comment), in the end I'm not sure yet that it's a good idea. The code is nicer, more specific and less case to handle. But, the error message in case of a class found as a parent of a constructor is less explicit: see the error message before and after in this case.

do you maybe see a fix for #932 at the same time ?

Not directly, but since this PR made me much more comfortable with this part of the code, I'll have a look!

Julow · 2023-03-30T09:08:34Z

But, the error message in case of a class found as a parent of a constructor is less explicit

This error message could perhaps be implemented in general ? The previous message is not satisfactory because it seems to be about t.A where it's actually about t:

Warning: Failed to resolve reference unresolvedroot(t).A is of kind class but expected signature or type

A new kind of error message is needed: "<parent> is of unexpected kind".

EmileTrotignon · 2023-12-05T13:21:28Z

I think this should be rebased, a better error message added, and then merged.

Co-authored-by: Paul-Elliot <peada@free.fr> Signed-off-by: Paul-Elliot <peada@free.fr>

This allows to omit the type the constructor is coming from. The constructor will be fetched from the environment. The commit does this in a similar way to fields, which can have type `parent` for parents of type references. Co-authored-by: Paul-Elliot <peada@free.fr> Signed-off-by: Paul-Elliot <peada@free.fr>

Indeed, `parent` is for constructor and fields: they can only have signature or datatype as parent. Signed-off-by: Paul-Elliot <peada@free.fr> Co-authored-by: Emile Trotignon <emile@tarides.com>

Resolved references/ids already always had datatype as parent, but it is now enforced by the subtyping system. The reason we cannot symmetrically do the same for fields is the following: ```ocaml type t = .. type t += Inline of { a : int} ``` In this example, `a` cannot have a `datatype` as parent. Parent for fields should ideally be `[datatype | constructor | extension]`. Signed-off-by: Paul-Elliot <peada@free.fr> Co-authored-by: Emile Trotignon <emile@tarides.com>

The former was too generic. Added some comments at the definition. Signed-off-by: Paul-Elliot <peada@free.fr>

since they are not used in resolved ref and id of cosntructor: for those, the parent is a type. Signed-off-by: Paul-Elliot <peada@free.fr>

To match the semantics of references Signed-off-by: Paul-Elliot <peada@free.fr>

This changes the error message, from "wrong type", to unresolved. Signed-off-by: Paul-Elliot <peada@free.fr>

Signed-off-by: Paul-Elliot <peada@free.fr>

panglesd · 2023-12-06T09:14:05Z

This is rebased, but I did not implement a better error message. Do you think it could be implemented in another PR? From what I remember from March 30, it is some non-negligible amount of work, but I don't remember why.

Julow

The error message is a secondary issue and should be discussed separately. This is good to merge :)

@panglesd

CHANGES: ### Added - Add support for external search engines (@panglesd, @EmileTrotignon, ocaml/odoc#972) This includes the generation of an index and the display of the results in the UI (HTML only). - Display 'private' keyword for private type extensions (@gpetiot, ocaml/odoc#1019) - Allow to omit parent type in constructor reference (@panglesd, @EmileTrotignon, ocaml/odoc#933) ### Fixed - Warn and exit when table(s) is not closed (@lubegasimon, ocaml/odoc#1050) - Hint when list(s) is not closed (@lubegasimon, ocaml/odoc#1050) - Fix crash on functors returning an alias (@Julow, ocaml/odoc#1046) - Fix rendering of polymorphic variants (@wikku, @panglesd, ocaml/odoc#971) - Add references to extension declarations (@gpetiot, @panglesd, ocaml/odoc#949) ### Changed - Style: Adjusted line height in the TOC to improve readability (@sorawee, ocaml/odoc#1045) - Style: Remove font fallback to Helvetica, Arial (@Julow, ocaml/odoc#1028) - Style: Preformatted elements fallback to UA monospace (@toastal, ocaml/odoc#967) - Style: Sidebar is now stuck to the left of the content instead of the left of the viewport (@EmileTrotignon, ocaml/odoc#999)

@panglesd

CHANGES: ### Added - Add support for external search engines (@panglesd, @EmileTrotignon, ocaml/odoc#972) This includes the generation of an index and the display of the results in the UI (HTML only). - Display 'private' keyword for private type extensions (@gpetiot, ocaml/odoc#1019) - Allow to omit parent type in constructor reference (@panglesd, @EmileTrotignon, ocaml/odoc#933) ### Fixed - Warn and exit when table(s) is not closed (@lubegasimon, ocaml/odoc#1050) - Hint when list(s) is not closed (@lubegasimon, ocaml/odoc#1050) - Fix crash on functors returning an alias (@Julow, ocaml/odoc#1046) - Fix rendering of polymorphic variants (@wikku, @panglesd, ocaml/odoc#971) - Add references to extension declarations (@gpetiot, @panglesd, ocaml/odoc#949) ### Changed - Style: Adjusted line height in the TOC to improve readability (@sorawee, ocaml/odoc#1045) - Style: Remove font fallback to Helvetica, Arial (@Julow, ocaml/odoc#1028) - Style: Preformatted elements fallback to UA monospace (@toastal, ocaml/odoc#967) - Style: Sidebar is now stuck to the left of the content instead of the left of the viewport (@EmileTrotignon, ocaml/odoc#999)

@panglesd

CHANGES: ### Added - Add support for external search engines (@panglesd, @EmileTrotignon, ocaml/odoc#972) This includes the generation of an index and the display of the results in the UI (HTML only). - Display 'private' keyword for private type extensions (@gpetiot, ocaml/odoc#1019) - Allow to omit parent type in constructor reference (@panglesd, @EmileTrotignon, ocaml/odoc#933) ### Fixed - Warn and exit when table(s) is not closed (@lubegasimon, ocaml/odoc#1050) - Hint when list(s) is not closed (@lubegasimon, ocaml/odoc#1050) - Fix crash on functors returning an alias (@Julow, ocaml/odoc#1046) - Fix rendering of polymorphic variants (@wikku, @panglesd, ocaml/odoc#971) - Add references to extension declarations (@gpetiot, @panglesd, ocaml/odoc#949) ### Changed - Style: Adjusted line height in the TOC to improve readability (@sorawee, ocaml/odoc#1045) - Style: Remove font fallback to Helvetica, Arial (@Julow, ocaml/odoc#1028) - Style: Preformatted elements fallback to UA monospace (@toastal, ocaml/odoc#967) - Style: Sidebar is now stuck to the left of the content instead of the left of the viewport (@EmileTrotignon, ocaml/odoc#999)

@panglesd

CHANGES: ### Added - Add support for external search engines (@panglesd, @EmileTrotignon, ocaml/odoc#972) This includes the generation of an index and the display of the results in the UI (HTML only). - Display 'private' keyword for private type extensions (@gpetiot, ocaml/odoc#1019) - Allow to omit parent type in constructor reference (@panglesd, @EmileTrotignon, ocaml/odoc#933) ### Fixed - Warn and exit when table(s) is not closed (@lubegasimon, ocaml/odoc#1050) - Hint when list(s) is not closed (@lubegasimon, ocaml/odoc#1050) - Fix crash on functors returning an alias (@Julow, ocaml/odoc#1046) - Fix rendering of polymorphic variants (@wikku, @panglesd, ocaml/odoc#971) - Add references to extension declarations (@gpetiot, @panglesd, ocaml/odoc#949) ### Changed - Style: Adjusted line height in the TOC to improve readability (@sorawee, ocaml/odoc#1045) - Style: Remove font fallback to Helvetica, Arial (@Julow, ocaml/odoc#1028) - Style: Preformatted elements fallback to UA monospace (@toastal, ocaml/odoc#967) - Style: Sidebar is now stuck to the left of the content instead of the left of the viewport (@EmileTrotignon, ocaml/odoc#999)

@panglesd

CHANGES: ### Added - Add support for external search engines (@panglesd, @EmileTrotignon, ocaml/odoc#972) This includes the generation of an index and the display of the results in the UI (HTML only). - Display 'private' keyword for private type extensions (@gpetiot, ocaml/odoc#1019) - Allow to omit parent type in constructor reference (@panglesd, @EmileTrotignon, ocaml/odoc#933) ### Fixed - Warn and exit when table(s) is not closed (@lubegasimon, ocaml/odoc#1050) - Hint when list(s) is not closed (@lubegasimon, ocaml/odoc#1050) - Fix crash on functors returning an alias (@Julow, ocaml/odoc#1046) - Fix rendering of polymorphic variants (@wikku, @panglesd, ocaml/odoc#971) - Add references to extension declarations (@gpetiot, @panglesd, ocaml/odoc#949) ### Changed - Style: Adjusted line height in the TOC to improve readability (@sorawee, ocaml/odoc#1045) - Style: Remove font fallback to Helvetica, Arial (@Julow, ocaml/odoc#1028) - Style: Preformatted elements fallback to UA monospace (@toastal, ocaml/odoc#967) - Style: Sidebar is now stuck to the left of the content instead of the left of the viewport (@EmileTrotignon, ocaml/odoc#999)

Julow approved these changes Mar 3, 2023

View reviewed changes

panglesd force-pushed the fix_447 branch 2 times, most recently from 3164be7 to a834b52 Compare March 30, 2023 06:36

panglesd force-pushed the fix_447 branch from a834b52 to b1f8292 Compare March 30, 2023 06:46

panglesd force-pushed the fix_447 branch from b1f8292 to abfe27d Compare March 30, 2023 07:07

panglesd mentioned this pull request Mar 31, 2023

Type references can resolve to classes #530

Closed

EmileTrotignon added this to the 2.4.0 milestone Dec 5, 2023

EmileTrotignon and others added 5 commits December 6, 2023 09:38

Add test for omitting parent type in constructor reference (ocaml#447)

7b66c13

Co-authored-by: Paul-Elliot <peada@free.fr> Signed-off-by: Paul-Elliot <peada@free.fr>

move class_signature_pv from parent to label_parent

be87d3a

Indeed, `parent` is for constructor and fields: they can only have signature or datatype as parent. Signed-off-by: Paul-Elliot <peada@free.fr> Co-authored-by: Emile Trotignon <emile@tarides.com>

rename parent into fragment_type_parent

bed9ef2

The former was too generic. Added some comments at the definition. Signed-off-by: Paul-Elliot <peada@free.fr>

panglesd force-pushed the fix_447 branch from abfe27d to 660daac Compare December 6, 2023 08:54

renaming fragment_type_parent to field_parent in resolved ref and id

725d900

since they are not used in resolved ref and id of cosntructor: for those, the parent is a type. Signed-off-by: Paul-Elliot <peada@free.fr>

panglesd force-pushed the fix_447 branch from 660daac to 3c90fe0 Compare December 6, 2023 09:06

panglesd added 4 commits December 6, 2023 10:07

invert type and datatype in Component

c2228c0

To match the semantics of references Signed-off-by: Paul-Elliot <peada@free.fr>

do not look for class when resolving a fragment_type_parent

b637501

This changes the error message, from "wrong type", to unresolved. Signed-off-by: Paul-Elliot <peada@free.fr>

Improve test for omitted type in constructor parent

c2c16a4

Signed-off-by: Paul-Elliot <peada@free.fr>

Changelog entry for ocaml#933

3c90fe0

Signed-off-by: Paul-Elliot <peada@free.fr>

Julow approved these changes Dec 6, 2023

View reviewed changes

Julow merged commit 5a054ff into ocaml:master Dec 6, 2023
5 checks passed

Julow mentioned this pull request Dec 11, 2023

[new release] odoc and odoc-parser (2.4.0) ocaml/opam-repository#24927

Closed

Julow mentioned this pull request Dec 12, 2023

[new release] odoc and odoc-parser (2.4.0) ocaml/opam-repository#24930

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow to omit parent type in constructor reference. #933

Allow to omit parent type in constructor reference. #933

panglesd commented Mar 2, 2023

Julow left a comment

Julow Mar 3, 2023

Julow Mar 3, 2023

panglesd Mar 30, 2023

dbuenzli commented Mar 30, 2023

panglesd commented Mar 30, 2023

Julow commented Mar 30, 2023

EmileTrotignon commented Dec 5, 2023

panglesd commented Dec 6, 2023

Julow left a comment

Allow to omit parent type in constructor reference. #933

Allow to omit parent type in constructor reference. #933

Conversation

panglesd commented Mar 2, 2023

Julow left a comment

Choose a reason for hiding this comment

Julow Mar 3, 2023

Choose a reason for hiding this comment

Julow Mar 3, 2023

Choose a reason for hiding this comment

panglesd Mar 30, 2023

Choose a reason for hiding this comment

dbuenzli commented Mar 30, 2023

panglesd commented Mar 30, 2023

Julow commented Mar 30, 2023

EmileTrotignon commented Dec 5, 2023

panglesd commented Dec 6, 2023

Julow left a comment

Choose a reason for hiding this comment