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

Import statements with module instantiation should not trigger an error message #2579

Closed
nad opened this issue May 10, 2017 · 9 comments
Closed
Assignees
Labels
instance Instance resolution modules Issues relating to the module system type: bug Issues and pull requests about actual bugs
Milestone

Comments

@nad
Copy link
Contributor

nad commented May 10, 2017

An import statement of the form import M args triggers the following error message:

An import statement with module instantiation does not actually import the module.  This statement achieves nothing.  Either add the `open' keyword or bind the instantiated module with an `as' clause.

However, if M contains instances, then this kind of import statement actually makes sense.

@nad nad added type: bug Issues and pull requests about actual bugs instance Instance resolution modules Issues relating to the module system labels May 10, 2017
@nad nad added this to the 2.5.3 milestone May 10, 2017
@andreasabel andreasabel self-assigned this May 13, 2017
@andreasabel
Copy link
Member

Ok, I commented out the line in the parser that raised the error.

UlfNorell added a commit that referenced this issue Dec 6, 2018
also fixes #1913 and rolls back change from #2579 allowing
import instantiation without `open` or `as`
@UlfNorell
Copy link
Member

I put back the check. With the fixes to #1913 and #2489 it no longer makes sense to not open or name the module.

@nad
Copy link
Contributor Author

nad commented Dec 7, 2018

Please document this change.

@nad nad reopened this Dec 7, 2018
@nad
Copy link
Contributor Author

nad commented Dec 17, 2018

Previously I could write

import M args

in order to bring instances into scope. Now I have to write something like

import M args as Dummy

Can someone please motivate this change?

@nad
Copy link
Contributor Author

nad commented Dec 17, 2018

The situation would be slightly better if I could write _ instead of inventing dummy names:

import M args as _

However, this doesn't work (#3457).

@nad nad reopened this Dec 17, 2018
@nad nad modified the milestones: 2.5.3, 2.6.0 Dec 17, 2018
@UlfNorell
Copy link
Member

Can someone please motivate this change?

You have filed most of the "instances should only be used if in scope" issues (for instance #1265 and #1913). Clearly if you just say import M args (or import M args as _) they wouldn't be in scope.

@nad
Copy link
Contributor Author

nad commented Dec 17, 2018

OK, I think that's a step in the right direction.

I do think that the following code is too ugly:

import M args as Dummy₁
import N args as Dummy₂

However, the reason that I have modules with nothing but instances is that instances became available for instance resolution so easily before. I wonder if I would have used a different design if the new rules had been in place when I wrote my code. (I don't feel like redesigning it now.)

@jespercockx
Copy link
Member

Wait, now I'm confused. Doesn't 'in scope' for instance arguments also include things that are only in scope under their qualified name? So then import M args should make all instances in M available for instance search? Quoting @UlfNorell from #1265:

In your example, however, the instance is in scope (as Instance.r). It's
quite common that you use modules qualified to avoid polluting the namespace.
It would be annoying if you were forced to make all instances available in the
top-level scope for them to be used.

(#1913 is about private instances which are really not in scope, not even under a qualified name.)

@jespercockx
Copy link
Member

Oh, I misunderstood. This issue is only about modules that are applied to arguments at the place where they're imported. I anyway never parametrize the top-level module in a file so I don't care much about how this works.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
instance Instance resolution modules Issues relating to the module system type: bug Issues and pull requests about actual bugs
Projects
None yet
Development

No branches or pull requests

4 participants