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

Reject ambiguous module imports. #4495

Closed
wants to merge 3 commits into from
Closed

Conversation

mbaulch
Copy link
Contributor

@mbaulch mbaulch commented Jul 17, 2016

Fixes #4485.

@Araq
Copy link
Member

Araq commented Jul 17, 2016

Filenames need to be unique per Nimble-package. And one test now fails with:

module stdlib/system is already defined in /home/travis/build/nim-lang/Nim/lib/system.nim

@mbaulch
Copy link
Contributor Author

mbaulch commented Jul 17, 2016

The way Nimble operates shouldn't, AFAIK, have any bearing on the compiler. The failing test relies on nimscript (compiler/scriptconfig.nim). I'll look for a sensible way to check whether nimscript is affecting module imports, and ignore the fake system/os packages it provides. Do you think that might work?

Not very knowledgeable about Nimble or Nimscript I confess.

@mbaulch
Copy link
Contributor Author

mbaulch commented Jul 17, 2016

There we are. Done.

@Araq
Copy link
Member

Araq commented Jul 17, 2016

Sorry, but this is convoluted and wrong (Nimscript can cause the reload any kind of module really, not only system.nim).

Every skModule symbol has an skPackage owner, why not store the list of known modules in this skPackage and report if there is an ambiguity arising?

@mbaulch
Copy link
Contributor Author

mbaulch commented Jul 17, 2016

I agree it's convoluted. Thanks for the pointer about Nimscript. That's important!

Will open a new PR, as that fix will have little in common with this one.

@mbaulch mbaulch closed this Jul 17, 2016
@mbaulch
Copy link
Contributor Author

mbaulch commented Jul 17, 2016

Sorry, I was a bit hasty with closing. Each skModule has a unique skPackage owner. We'd have to keep a list of skPackage representatives (PSym), and point each skModule's owner to the correct one. I can't see that such a list already exists, does it?

Your point about Nimscript (and modules besides system.nim getting reloaded) still stands, of course.

@mbaulch mbaulch reopened this Jul 17, 2016
@mbaulch
Copy link
Contributor Author

mbaulch commented Jul 18, 2016

Any module can now be reloaded. I think we're okay now.

@Araq
Copy link
Member

Araq commented Jul 19, 2016

Fixed it differently, sorry. :-)

@Araq Araq closed this Jul 19, 2016
@mbaulch
Copy link
Contributor Author

mbaulch commented Jul 19, 2016

That's okay. Your fix 9eb909b is more comprehensive, and possibly more efficient too.

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