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

Fix references to shadowed items #682

Merged
merged 3 commits into from
Jun 9, 2021
Merged

Conversation

Julow
Copy link
Collaborator

@Julow Julow commented May 27, 2021

Fixes #648

This PR implements shadowing when resolving submodules. Before, several items with the same name were added to the environment, which caused the warnings.

The notion of scope didn't change: the whole signature is added to the environment before any reference is resolved (including opens and includes).

Julow added 2 commits May 27, 2021 18:33
The output for 'shadowed.mli' is right but the warnings shouldn't occur.
The output for 'shadowed_through_open.mli' is wrong.
Before this patch, Odoc_xref2.Env doesn't implement shadowing and
several elements of the same kind with the same name could be
represented.
@@ -1,5 +1,25 @@
A quick test to repro the issue found in #587

$ ./build.sh
File "odoc_bug__a_intf.cmt":
Copy link
Member

Choose a reason for hiding this comment

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

What's going on here? I don't understand why this would start failing now?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed. I misunderstood lookup by identifier in the environment. The previous code was removing shadowed items from the environment but we need to keep them in the case of aliases to the same name. We only need to remove them from "lookup by name" results.

Shadowing should only be implemented when looking up by name.
This is important for aliases to an item of the same name.
@jonludlam
Copy link
Member

The fastly outage is causing the test failures IFAICT

@Julow
Copy link
Collaborator Author

Julow commented Jun 8, 2021

Most CI jobs passed :)

@jonludlam jonludlam merged commit 92b3f1d into ocaml:master Jun 9, 2021
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.

Warnings about ambigous references are bogus
2 participants