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

Bug fixes #853

Merged
merged 11 commits into from
Jun 14, 2022
Merged

Bug fixes #853

merged 11 commits into from
Jun 14, 2022

Conversation

jonludlam
Copy link
Member

A set of fairly self-contained bug fixes found over the past few weeks.

For signatures like:

    sig
      module M := N
      type t = M.t
    end

we now render this as written rather than as

    sig
      module M := N
      type t = N.t
    end

I think this makes more sense (otherwise why render the
`module M := N` at all?)
This is analogous logic that was already present in the handling of
cmi files.
The test was previously incorrect. An additional fix was also needed
in the code for handling canonical modules, which was relying on the
previous wrong behaviour.
Introduces a currently failing case where a shadowed identifier is
created with a non-unique name.
Ensures that when including a signature that contains shadowed
elements, these are refreshed to make sure the identifiers continue
to be unique
Prior to this commit, the contents of included structs or sigs were
inlined into the parent signature. This led to problems when there
was shadowing happening.

After this commit we need to be careful not to do twice as much work
as necessary, since there are now _two_ `Signature.t` records in
an include like this.
This should never happen, but it does. Further bug fixing required.
We don't use canonical on references, we assume the user will write
the correct path. This commit removes the unused constructor.
Copy link
Collaborator

@Julow Julow left a comment

Choose a reason for hiding this comment

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

Nice!

src/xref2/env.ml Show resolved Hide resolved
src/xref2/env.ml Show resolved Hide resolved
test/generators/html/Recent-X.html Show resolved Hide resolved
in
match status with
| `Inline -> included_html
| `Closed -> details ~open':false
| `Open -> details ~open':true
| `Default -> details ~open':!Tree.open_details
in
let inc = [ Html.div ~a:[ Html.a_class a_class ] (doc @ content) ] in
(continue_with [@tailcall]) rest inc
(continue_with [@tailcall]) rest content
Copy link
Collaborator

Choose a reason for hiding this comment

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

We loose the comment attached to the include. (the code doc @ _ is not in every branches, relevant test is test/generators/html/Toplevel_comments-Include_inline'.html)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah! Good point thanks!

$ ocamlc -c -bin-annot -I . c.mli
$ ocamlc -c -bin-annot -I . d.mli
$ odoc compile -I . c.cmti
$ odoc compile -I . d.cmti
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should also look at the result with odoc_print.

Copy link
Member Author

Choose a reason for hiding this comment

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

Could do! I didn't put much thought into this - it was an example provided by Luke M that was failing in a nasty way, so I was satisfied that it didn't die horribly after this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

On reflection I don't think I'll do this at this point, that it doesn't crash horribly is sufficient for now.

Copy link
Collaborator

@Julow Julow left a comment

Choose a reason for hiding this comment

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

Let's merge !

@jonludlam jonludlam merged commit ea5021e into ocaml:master Jun 14, 2022
@jonludlam
Copy link
Member Author

In it goes :-)

mseri pushed a commit to ocaml/opam-repository that referenced this pull request Dec 13, 2022
CHANGES:

Additions
- New unstable option `--as-json` for the HTML renderer that emits HTML
  fragments (preamble, content) together with metadata (table of contents,
  breadcrumbs, whether katex is used) in JSON format. (@sabine, ocaml/odoc#908)
- New maths support via `{m ... }` and `{math ... }` tags. (@giltho, @gpetiot, ocaml/odoc#886)
- Various optimisations (@jonludlam, ocaml/odoc#870, ocaml/odoc#883)
- Better handling of alerts and deprecation notices. (@panglesd, ocaml/odoc#828)
- Handle language tags on code blocks (@Julow, ocaml/odoc#848)

Bugfixes
- Shadowing issues (@jonludlam, ocaml/odoc#853)
- Layout fixes and improvements (@panglesd, ocaml/odoc#832, ocaml/odoc#839, ocaml/odoc#847)
- Handle comments on class constraints and inherit (@Julow, ocaml/odoc#844)
- Disable the missing root warning (@jonludlam, ocaml/odoc#881)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants