-
Notifications
You must be signed in to change notification settings - Fork 102
Resolve some references in synopses in {!modules} lists
#658
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
Conversation
This is no longer used since parser tests were rewritten as expect-tests.
36ea0c9 to
bb406ea
Compare
| {"`Root":["M","`TUnknown"]} | ||
| {"`Dot":[{"`Root":["M","`TUnknown"]},"t"]} | ||
| {"`Resolved":{"`Identifier":{"`Root":[{"`RootPage":"test"},"M"]}}} | ||
| {"`Resolved":{"`Type":[{"`Identifier":{"`Root":[{"`RootPage":"test"},"M"]}},"t"]}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This changed because I've added back the missing M module. I think that's how the test was intended to be.
|
|
||
| module Resolve_synopsis : sig | ||
| (** This should be resolved when included: {!Main.Resolve_synopsis.t}. These | ||
| shouldn't: {!t} {!Resolve_synopsis.t} *) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be easy to resolve these two references:
- Add the target module to the new env
- Open it
How sound it is ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's leave this as an issue to resolve later - we need to discuss the various options available to us.
|
I implemented best-effort resolving of fully-qualified references encountered in the synopsis. There were no error reported when a reference failed to resolve so I've added one and attached some information to it when it happens while resolving a synopsis. The result is quite verbose but don't trigger fatal warnings, it'll need to be improved later. (errors need to be propagated while resolving the references) |
Attempt to resolve references in synopses using a empty environment. Fully-qualified references should resolve. Removes the previous warning when a reference was encountered.
bb406ea to
31da4fb
Compare
|
I've removed the warning code. (#664) |
{!modules} lists{!modules} lists
These references need to be resolved from their containing module's environment, which is only available when linking that module.
This PR adds a warning in case a reference is found and doesn't try to resolve it.
It should be possible to resolve some fully-qualified references using the available environment as a best-effort and to resolve references local to their parent module. That PR doesn't try that.
Rebased on top of #654