Skip to content
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.

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

Remove core types and exceptions from identifiers #1242

Merged
merged 5 commits into from
Nov 15, 2024

Conversation

panglesd
Copy link
Collaborator

Identifiers are values that can be turned into a link. Core types and values cannot, so they should not have an ID.

This PR fixes this. Core types can appear in unresolved path, and that's all. This better representation allows to:

  • Have Url.from_identifier not return a result type (which removes many assert false from the code!)
  • Remove the Predefined module, which did not made sense.

They are not used anywhere and do not belong to identifiers: we cannot generate
a link from them.
Instead, put them into (unresolved) path types.
@panglesd panglesd added the no changelog This pull request does not need a changelog entry label Nov 12, 2024
Copy link
Collaborator

@gpetiot gpetiot left a comment

Choose a reason for hiding this comment

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

This looks good to me, the code simplification is impressive!
Do you have an idea what causes the mdx issue in the tests?

@panglesd
Copy link
Collaborator Author

Yes: the mdx tests are only run on dune build @runmdx, and I forgot to do that. The new output is legit, I promoted them.

@jonludlam
Copy link
Member

I'm not convinced this is the right approach, though it's not without its merits. Perhaps we can try removing core types only from identifiers instead?

@panglesd
Copy link
Collaborator Author

I tried your suggested approach: moving "path to core types" from identifiers to resolved paths. So paths to core type are represented by:

  • `Resolved (`Identifier (`CoreType name)) before this PR
  • `CoreType name before @jonludlam's suggestion
  • `Resolved (`CoreType name) after the suggestion

As a consequence of considering core types path as resolved, taking an identifier out of a path type returns an option.
However, this is likely better if we, one day, want to add canonical references to core types.

@gpetiot
Copy link
Collaborator

gpetiot commented Nov 15, 2024

However, this is likely better if we, one day, want to add canonical references to core types.

You mean so that we can link to e.g. https://github.com/ocaml/ocaml/blob/trunk/stdlib/bool.mli#L22 each time there is a bool, or is there other benefits to having these references? is it useful for the search?

Copy link
Collaborator

@gpetiot gpetiot left a comment

Choose a reason for hiding this comment

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

The new diff looks good by the way.

@panglesd
Copy link
Collaborator Author

You mean so that we can link to e.g. https://github.com/ocaml/ocaml/blob/trunk/stdlib/bool.mli#L22 each time there is a bool, or is there other benefits to having these references? is it useful for the search?

Sorry, I did not meant references but paths.

module Int : sig
  type t = int
  (** @canonical int *)
end

val plus_one : Int.t -> Int.t

If the feature I was mentioning were implemented, it would make the rendered signature of plus_one use the core type int instead of the alias Int.t.

@jonludlam
Copy link
Member

This is a very nice change now!

@jonludlam jonludlam merged commit 20f28f5 into ocaml:master Nov 15, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no changelog This pull request does not need a changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants