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

Strange undefined global error on trunk #7144

Closed
vicuna opened this issue Feb 10, 2016 · 7 comments
Closed

Strange undefined global error on trunk #7144

vicuna opened this issue Feb 10, 2016 · 7 comments
Assignees

Comments

@vicuna
Copy link

vicuna commented Feb 10, 2016

Original bug ID: 7144
Reporter: @Drup
Assigned to: @garrigue
Status: resolved (set by @garrigue on 2017-03-14T07:04:11Z)
Resolution: not a bug
Priority: normal
Severity: minor
Target version: undecided
Category: typing
Monitored by: @Drup @hcarty

Bug description

I have been able to trigger a non-sensical "undefined global" error on trunk with the following setup:

In a file A, I have a module alias
module P = Parsetree
which is the only direct dependency from A to Parsetree (all uses are from the module alias).
In a file B, I have a dependency to A and a transitive or direct dependency to Parsetree, but without any alias.

I get the error
Error: Error while linking foo.cma(A):
Reference to undefined global `Parsetree'

I have not been able to create a minimal example but I have an easily reproducible test case:

Given a freshly compiled trunk source tree
If you add "let _f x = Parsetree.Pexp_ident x" at the top of typing/ident.ml
"make depend && make world" and it works
If you add "module P = Parsetree" the same steps don't work.

Playing with .depend doesn't help. I have also another project using compiler-libs that exhibits the issue.

@vicuna
Copy link
Author

vicuna commented Feb 10, 2016

Comment author: @Drup

Side note: The choice of Ident is purely artificial, it should work with any module that doesn't already have a dependency to Parsetree.

@vicuna
Copy link
Author

vicuna commented Feb 10, 2016

Comment author: @diml

This is because parsetree only has a .mli. You can refer to its types but can't create a reference to its implementation. Same goes for asttypes

@vicuna
Copy link
Author

vicuna commented Feb 10, 2016

Comment author: @Drup

@diml: I see, so this is the cause (and it explains why I couldn't reproduce, I was trying with List ...), but there is still a bug.

On trunk, this works in the toplevel:

#require "compiler-libs.common" ;;
module P = Parsetree ;;
let x = P.Pexp_ident (Location.mknoloc (Longident.Lident "foo")) ;;

But not in a file.

@vicuna
Copy link
Author

vicuna commented Feb 10, 2016

Comment author: @lpw25

I suspect this is related to the special handling of module aliases that Jacques added to make sure we didn't break backwards compatibility by failing to link modules that are only referred to via an alias (unless you use -no-alias-deps). It's possible that it does not do the "does this module only contain types" check which allows modules with only .mli files to work. It's also possible that it doesn't affect the toplevel.

@vicuna
Copy link
Author

vicuna commented Feb 11, 2016

Comment author: @garrigue

Are you sure this is a regression?
As dim says, the problem is that you cannot create a reference to the implementation, and as lpw25 points, there is code to ensure backwards compatibility that introduces a reference for module aliases, but I believe it does not introduce more references than before.

The reason this works in the toplevel is that it does not use the same code path, and as a result does not introduce the extra references. This can be seen as bug, but this is far from being the only difference between the toplevel and linked code. Since this is rather low priority, modifying the toplevel code to introduce the extra references will have to wait for after 4.03.

Note that in 4.02 the references were introduced, so your code did fail in the toplevel. But in 4.03 these references are merged with the new mechanism that introduces references when using primitives, and this mechanism is not activated in the toplevel.

@vicuna
Copy link
Author

vicuna commented Feb 16, 2017

Comment author: @xavierleroy

In the end I didn't understand if something needs to be changed or not. If not please "resolve" this report. If so, let's assign this PR to someone and give a target version.

@vicuna
Copy link
Author

vicuna commented Mar 14, 2017

Comment author: @garrigue

Marking it as resolved, since there were no reactions.
It doesn't seem worth adding extra code just to avoid introducing a dependency in some special case (all the more as you can remove the dependency by using the -no-alias-deps option.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants