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

Permit a single open import without a warning #1869

Closed
tfausak opened this Issue Feb 6, 2016 · 92 comments

Comments

Projects
None yet
@tfausak
Contributor

tfausak commented Feb 6, 2016

Or: "What happened to (..)?"

Let's say I have a module B that should reexport everything from A, like so:

-- A.purs
module A where data X
-- B.purs
module B (module A) where import A

Compiling that with PureScript 0.8.0, I get the following warning:

Warning found:
in module B
at /private/tmp/example/src/B.purs line 1, column 27 - line 1, column 34

  Module A has unspecified imports, consider using the explicit form:

    import A (X)



See https://github.com/purescript/purescript/wiki/Error-Code-ImplicitImport for more information,
or to contribute content related to this warning.

I used to be able to explicitly get open imports by saying import A (..), but that's a syntax error now.

-- B.purs
module B (module A) where import A (..)
Error found:
at /private/tmp/example/src/B.purs line 1, column 37 - line 1, column 37

  Unable to parse module:
  unexpected ..
  expecting identifier, (, proper name, "class", "module" or )


See https://github.com/purescript/purescript/wiki/Error-Code-ErrorParsingModule for more information,
or to contribute content related to this error.

What's the right way to do this?

@natefaubion

This comment has been minimized.

Show comment
Hide comment
@natefaubion

natefaubion Feb 6, 2016

Contributor

The (..) syntax was removed. The warnings are so to strongly discourage open imports of any kind. When open imports are allowed, adding a function or definition to exports has to be a breaking change, because it can potentially cause clashes with downstream libraries.

For example:

module A where
  data Foo

module B where
  data Bar

module C where
  import A
  import B

As the author of library A, maybe I'll decide to add an export for a new internal data structure that I want to make public, and it happens to be called Bar. I would not consider this a breaking change, since I'm only adding to the exported API. So I release it with a patch version. But downstream module C is using open imports, and updates to the latest patch release and all of a sudden their code is broken :(

The answer is just to list all your imports.

Contributor

natefaubion commented Feb 6, 2016

The (..) syntax was removed. The warnings are so to strongly discourage open imports of any kind. When open imports are allowed, adding a function or definition to exports has to be a breaking change, because it can potentially cause clashes with downstream libraries.

For example:

module A where
  data Foo

module B where
  data Bar

module C where
  import A
  import B

As the author of library A, maybe I'll decide to add an export for a new internal data structure that I want to make public, and it happens to be called Bar. I would not consider this a breaking change, since I'm only adding to the exported API. So I release it with a patch version. But downstream module C is using open imports, and updates to the latest patch release and all of a sudden their code is broken :(

The answer is just to list all your imports.

@natefaubion

This comment has been minimized.

Show comment
Hide comment
@natefaubion

natefaubion Feb 6, 2016

Contributor

That said, it is really obnoxious with Prelude type modules. Prelude rarely changes, and if any of the API were to change it should at least be a minor bump.

Contributor

natefaubion commented Feb 6, 2016

That said, it is really obnoxious with Prelude type modules. Prelude rarely changes, and if any of the API were to change it should at least be a minor bump.

@tfausak

This comment has been minimized.

Show comment
Hide comment
@tfausak

tfausak Feb 6, 2016

Contributor

I understand what you're saying, but that only really applies when you have both open exports and imports. I didn't specify when I created this issue because it wasn't necessary to recreate, but I typically use explicit exports rather than imports. For example, Neon.Class exports 28 other modules. Each of those, like Neon.Class.Add, explicitly list their exports. Doing it in both places is a huge pain for me and not something I want to do.

Contributor

tfausak commented Feb 6, 2016

I understand what you're saying, but that only really applies when you have both open exports and imports. I didn't specify when I created this issue because it wasn't necessary to recreate, but I typically use explicit exports rather than imports. For example, Neon.Class exports 28 other modules. Each of those, like Neon.Class.Add, explicitly list their exports. Doing it in both places is a huge pain for me and not something I want to do.

@natefaubion

This comment has been minimized.

Show comment
Hide comment
@natefaubion

natefaubion Feb 6, 2016

Contributor

I understand what you're saying, but that only really applies when you have both open exports and imports.

That's not quite true. I think for psc to make that distinction it would have to have some notion of the packages (packages should be able to openly import from themselves because otherwise it wouldn't have compiled in the first place). As far as psc is concerned, each module could be coming from completely unrelated packages, and is therefor unsafe to do so.

Contributor

natefaubion commented Feb 6, 2016

I understand what you're saying, but that only really applies when you have both open exports and imports.

That's not quite true. I think for psc to make that distinction it would have to have some notion of the packages (packages should be able to openly import from themselves because otherwise it wouldn't have compiled in the first place). As far as psc is concerned, each module could be coming from completely unrelated packages, and is therefor unsafe to do so.

@michaelficarra

This comment has been minimized.

Show comment
Hide comment
@michaelficarra

michaelficarra Feb 6, 2016

Contributor

@tfausak That's not at all the case. The problem arises whenever you have open imports, regardless of your exports.

Contributor

michaelficarra commented Feb 6, 2016

@tfausak That's not at all the case. The problem arises whenever you have open imports, regardless of your exports.

@tfausak

This comment has been minimized.

Show comment
Hide comment
@tfausak

tfausak Feb 6, 2016

Contributor

Y'all are right, of course. I am considering this from the perspective of a single library. When you get other libraries involved, the case explained by @natefaubion is easily possible.

Within a package, explicitly listing exports and imports is annoying.

Contributor

tfausak commented Feb 6, 2016

Y'all are right, of course. I am considering this from the perspective of a single library. When you get other libraries involved, the case explained by @natefaubion is easily possible.

Within a package, explicitly listing exports and imports is annoying.

@garyb

This comment has been minimized.

Show comment
Hide comment
@garyb

garyb Feb 6, 2016

Member

It is a bit, but that's why the warning prints the fix.

Member

garyb commented Feb 6, 2016

It is a bit, but that's why the warning prints the fix.

@michaelficarra

This comment has been minimized.

Show comment
Hide comment
@michaelficarra

michaelficarra Feb 6, 2016

Contributor

@tfausak You can choose to qualify your imports instead: import A as A. Hopefully in the future when we have no more pre-0.8 code around, we can change import A to be a shorthand for that.

Contributor

michaelficarra commented Feb 6, 2016

@tfausak You can choose to qualify your imports instead: import A as A. Hopefully in the future when we have no more pre-0.8 code around, we can change import A to be a shorthand for that.

@tfausak

This comment has been minimized.

Show comment
Hide comment
@tfausak

tfausak Feb 6, 2016

Contributor

I find the warning unsatisfactory because if I want to add something to Neon.Class.Add I have to add it to both the export list and the import list in Neon.Class.

The qualified import is a good trick. I know HLint usually recommends doing import A.B.C as X so that your export list only has a single module X in it. I typically don't do that because it confuses Haddock, but I haven't checked what psc-publish generates for that.

Contributor

tfausak commented Feb 6, 2016

I find the warning unsatisfactory because if I want to add something to Neon.Class.Add I have to add it to both the export list and the import list in Neon.Class.

The qualified import is a good trick. I know HLint usually recommends doing import A.B.C as X so that your export list only has a single module X in it. I typically don't do that because it confuses Haddock, but I haven't checked what psc-publish generates for that.

@garyb

This comment has been minimized.

Show comment
Hide comment
@garyb

garyb Feb 6, 2016

Member

I know what you mean. I don't like this either, but it's definitely the least bad solution we have right now. We have actually been bitten by upstream library changes breaking their dependants, if these warnings are heeded that will never happen, which seems like a worthwhile tradeoff.

Member

garyb commented Feb 6, 2016

I know what you mean. I don't like this either, but it's definitely the least bad solution we have right now. We have actually been bitten by upstream library changes breaking their dependants, if these warnings are heeded that will never happen, which seems like a worthwhile tradeoff.

@paf31

This comment has been minimized.

Show comment
Hide comment
@paf31

paf31 Feb 6, 2016

Member

And remember, you can always filter warnings using purescript-psa. The decision was made to not provide that functionality in the compiler, since we try to push these sorts of things into tools where possible.

Member

paf31 commented Feb 6, 2016

And remember, you can always filter warnings using purescript-psa. The decision was made to not provide that functionality in the compiler, since we try to push these sorts of things into tools where possible.

@tfausak

This comment has been minimized.

Show comment
Hide comment
@tfausak

tfausak Feb 6, 2016

Contributor

That's good to know. I would prefer to build my libraries without warnings, though.

Contributor

tfausak commented Feb 6, 2016

That's good to know. I would prefer to build my libraries without warnings, though.

@tfausak

This comment has been minimized.

Show comment
Hide comment
@tfausak

tfausak Feb 6, 2016

Contributor

I ran into a few more annoyances while trying to work around this.

  • The qualified import trick does not work if you want to re-export multiple modules. module A (module X) where import B as X; import C as X gives a warning.
  • The qualified import trick does not work if you hide imports. module A (module B) where import B hiding (X) as B gives a warning.
  • There is apparently no solution for importing a prelude style module without warnings. For instance, this spec imports a common Test.Helper module that is intended to be used with open imports. That gives a warning. It's annoying to work with (even with the copy-pastable code in the warning) because I'd have to change the import if I added or removed a pending spec, for example. (I don't want to import it qualified because I think qualified operators are hideous.)

I see I missed #1742, which removed the import A (..) syntax for open imports. I wish there was still a syntax for open imports. I can't think of another language that doesn't have them. Every example on the PureScript homepage and on Try PureScript! would emit at least one warning because of this.

Contributor

tfausak commented Feb 6, 2016

I ran into a few more annoyances while trying to work around this.

  • The qualified import trick does not work if you want to re-export multiple modules. module A (module X) where import B as X; import C as X gives a warning.
  • The qualified import trick does not work if you hide imports. module A (module B) where import B hiding (X) as B gives a warning.
  • There is apparently no solution for importing a prelude style module without warnings. For instance, this spec imports a common Test.Helper module that is intended to be used with open imports. That gives a warning. It's annoying to work with (even with the copy-pastable code in the warning) because I'd have to change the import if I added or removed a pending spec, for example. (I don't want to import it qualified because I think qualified operators are hideous.)

I see I missed #1742, which removed the import A (..) syntax for open imports. I wish there was still a syntax for open imports. I can't think of another language that doesn't have them. Every example on the PureScript homepage and on Try PureScript! would emit at least one warning because of this.

@hdgarrood

This comment has been minimized.

Show comment
Hide comment
@hdgarrood

hdgarrood Feb 6, 2016

Contributor

I'm beginning to think the answer could be to make the compiler aware of packages and to only emit the warning for modules defined in different packages.

Contributor

hdgarrood commented Feb 6, 2016

I'm beginning to think the answer could be to make the compiler aware of packages and to only emit the warning for modules defined in different packages.

@tfausak

This comment has been minimized.

Show comment
Hide comment
@tfausak

tfausak Feb 6, 2016

Contributor

That would be a good way to handle the scenario that I'm dealing with now. But how should you import the Prelude (or Neon) without a warning?

Contributor

tfausak commented Feb 6, 2016

That would be a good way to handle the scenario that I'm dealing with now. But how should you import the Prelude (or Neon) without a warning?

@garyb

This comment has been minimized.

Show comment
Hide comment
@garyb

garyb Feb 6, 2016

Member

Just to address one minor point: you could import operator(s) explicitly and then use a qualified import for the rest of the module.

Member

garyb commented Feb 6, 2016

Just to address one minor point: you could import operator(s) explicitly and then use a qualified import for the rest of the module.

@garyb

This comment has been minimized.

Show comment
Hide comment
@garyb

garyb Feb 6, 2016

Member

As for the other point, there is no safe way to allow open imports of any module, even if it is Prelude or Neon, unless we decide to start treating additions to a module as a breaking change.

Member

garyb commented Feb 6, 2016

As for the other point, there is no safe way to allow open imports of any module, even if it is Prelude or Neon, unless we decide to start treating additions to a module as a breaking change.

@tfausak

This comment has been minimized.

Show comment
Hide comment
@tfausak

tfausak Feb 6, 2016

Contributor

I don't think adding exports should be a breaking change. But it's also not a patch-level change, as @natefaubion originally suggested. It would be a minor version change, at least according to semver.

For what it's worth, I have never run into this particular problem. I see how it can happen, but it seems exceptionally rare.

Can this only be a warning if you reexport a module that you import openly?

Contributor

tfausak commented Feb 6, 2016

I don't think adding exports should be a breaking change. But it's also not a patch-level change, as @natefaubion originally suggested. It would be a minor version change, at least according to semver.

For what it's worth, I have never run into this particular problem. I see how it can happen, but it seems exceptionally rare.

Can this only be a warning if you reexport a module that you import openly?

@hdgarrood

This comment has been minimized.

Show comment
Hide comment
@hdgarrood

hdgarrood Feb 6, 2016

Contributor

Another thing that's just occurred to me - I think the usefulness of this warning depends on the code you're writing. For an application, you probably don't want it. For core libraries, you almost certainly do. So I think the answer may be to come up with a good set of defaults in psa; for example, I think this particular warning should be opt-in.

Contributor

hdgarrood commented Feb 6, 2016

Another thing that's just occurred to me - I think the usefulness of this warning depends on the code you're writing. For an application, you probably don't want it. For core libraries, you almost certainly do. So I think the answer may be to come up with a good set of defaults in psa; for example, I think this particular warning should be opt-in.

@garyb

This comment has been minimized.

Show comment
Hide comment
@garyb

garyb Feb 6, 2016

Member

We have run into this problem, that's why it's a concern 😄. Even if not, the fact that a minor version bump can break code without this warning makes it a concern.

The problem has nothing to do with what a module exports, it arises with open imports due to the possibility of multiple values with the same name being brought into the same scope. If you use explicit imports then this can never happen. If you import two modules qualified with the same name, they're also being imported into the same scope, which is why it is a warning to do so without being explicit about what you're importing for both too. Import hiding is another form of importing implicitly, as again it only specifically rejects some things while importing everything else.

The only possible situation in which it would be safe to import openly is: if the module has no members of its own, and only has one import. So basically a situation that is entirely useless.

Member

garyb commented Feb 6, 2016

We have run into this problem, that's why it's a concern 😄. Even if not, the fact that a minor version bump can break code without this warning makes it a concern.

The problem has nothing to do with what a module exports, it arises with open imports due to the possibility of multiple values with the same name being brought into the same scope. If you use explicit imports then this can never happen. If you import two modules qualified with the same name, they're also being imported into the same scope, which is why it is a warning to do so without being explicit about what you're importing for both too. Import hiding is another form of importing implicitly, as again it only specifically rejects some things while importing everything else.

The only possible situation in which it would be safe to import openly is: if the module has no members of its own, and only has one import. So basically a situation that is entirely useless.

@hdgarrood

This comment has been minimized.

Show comment
Hide comment
@hdgarrood

hdgarrood Feb 6, 2016

Contributor

@taylorfausak the problem with making additions a minor change and allowing open imports simultaneously is that a minor upgrade can break your package by introducing a name clash.

Contributor

hdgarrood commented Feb 6, 2016

@taylorfausak the problem with making additions a minor change and allowing open imports simultaneously is that a minor upgrade can break your package by introducing a name clash.

@garyb

This comment has been minimized.

Show comment
Hide comment
@garyb

garyb Feb 6, 2016

Member

@hdgarrood even that's not safe really, as if Prelude changes or whatever it could break code in your app, unless the app sticks to exact versions rather than ranges.

Member

garyb commented Feb 6, 2016

@hdgarrood even that's not safe really, as if Prelude changes or whatever it could break code in your app, unless the app sticks to exact versions rather than ranges.

@hdgarrood

This comment has been minimized.

Show comment
Hide comment
@hdgarrood

hdgarrood Feb 6, 2016

Contributor

Yes, but I'd still so much sooner have to periodically fix this sort of break while updating to new minor versions of my dependencies than have to fiddle with import lists almost every time I change my code. I also think sticking to exact versions is usually the right approach for applications anyway.

Contributor

hdgarrood commented Feb 6, 2016

Yes, but I'd still so much sooner have to periodically fix this sort of break while updating to new minor versions of my dependencies than have to fiddle with import lists almost every time I change my code. I also think sticking to exact versions is usually the right approach for applications anyway.

@tfausak

This comment has been minimized.

Show comment
Hide comment
@tfausak

tfausak Feb 6, 2016

Contributor

I'd still so much sooner have to periodically fix this sort of break while updating to new minor versions of my dependencies than have to fiddle with import lists almost every time I change my code.

👍 ✖️ 💯

Contributor

tfausak commented Feb 6, 2016

I'd still so much sooner have to periodically fix this sort of break while updating to new minor versions of my dependencies than have to fiddle with import lists almost every time I change my code.

👍 ✖️ 💯

@garyb

This comment has been minimized.

Show comment
Hide comment
@garyb

garyb Feb 6, 2016

Member

@tfausak he's talking about apps there though, not libraries.

Member

garyb commented Feb 6, 2016

@tfausak he's talking about apps there though, not libraries.

@tfausak

This comment has been minimized.

Show comment
Hide comment
@tfausak

tfausak Feb 6, 2016

Contributor

Fair enough, but I'm talking about libraries as well as apps.

Contributor

tfausak commented Feb 6, 2016

Fair enough, but I'm talking about libraries as well as apps.

@garyb

This comment has been minimized.

Show comment
Hide comment
@garyb

garyb Feb 6, 2016

Member

Yeah I know, but I don't think breaking dependants is an acceptable solution :)

Member

garyb commented Feb 6, 2016

Yeah I know, but I don't think breaking dependants is an acceptable solution :)

@tfausak

This comment has been minimized.

Show comment
Hide comment
@tfausak

tfausak Feb 6, 2016

Contributor

The main problem for me is that there's no way for me to convince the compiler that I know what I'm doing. I understand the warning, I understand the problems that can arise, and I want to do it anyway. Why can't I?

I see #1742 (comment) suggests making import X mean import X as X (i.e., making the default import qualified). I am fully in favor of that. In fact, I think Elm's imports are my favorite. But the key point to me right now is that it's possible to do an open import without warnings in PureScript.

Obviously the intent of warnings it to encourage people to fix them. Do you really want people writing import Prelude ((+), bind, etc) instead of import Prelude (..)?

Contributor

tfausak commented Feb 6, 2016

The main problem for me is that there's no way for me to convince the compiler that I know what I'm doing. I understand the warning, I understand the problems that can arise, and I want to do it anyway. Why can't I?

I see #1742 (comment) suggests making import X mean import X as X (i.e., making the default import qualified). I am fully in favor of that. In fact, I think Elm's imports are my favorite. But the key point to me right now is that it's possible to do an open import without warnings in PureScript.

Obviously the intent of warnings it to encourage people to fix them. Do you really want people writing import Prelude ((+), bind, etc) instead of import Prelude (..)?

@hdgarrood

This comment has been minimized.

Show comment
Hide comment
@hdgarrood

hdgarrood Feb 6, 2016

Contributor

Yeah, I was only talking about applications.

If we did make the compiler aware of packages, and then turned off the warning for open imports from the same package, you could then define an internal prelude module for your package which re-exports everything in the real Prelude (and maybe a couple of extras if you want to), and then you could have an open prelude-style module import without generating this warning, and I think that could alleviate most of the pain. Additionally, I think tooling which automatically adjusted import lists for you could help a lot too. Ideally I guess we would have both of these things.

Contributor

hdgarrood commented Feb 6, 2016

Yeah, I was only talking about applications.

If we did make the compiler aware of packages, and then turned off the warning for open imports from the same package, you could then define an internal prelude module for your package which re-exports everything in the real Prelude (and maybe a couple of extras if you want to), and then you could have an open prelude-style module import without generating this warning, and I think that could alleviate most of the pain. Additionally, I think tooling which automatically adjusted import lists for you could help a lot too. Ideally I guess we would have both of these things.

tfausak added a commit to tfausak/purescript-neon that referenced this issue Feb 6, 2016

@paf31 paf31 added this to the Discussion milestone Feb 6, 2016

@texastoland

This comment has been minimized.

Show comment
Hide comment
@texastoland

texastoland Feb 21, 2016

Where do I read to understand imports in PureScript? It seems like they're unqualified by default (whereas Elm's are qualified as pointed out by @tfausak), no longer provide syntax to use unspecified without warning, essentially pushing users to either qualify or specify, motivated to prevent the chance of future collision.

Clashing names is why I thought minor versions exist in semver. I don't see why code should be noisy to prevent a break resulting from a minor change any more than a major one. Where it's a concern I assume users don't import unspecified or limit updates to patch level.

If all those are correct I'm missing how to use future preludes without warnings and how PureScript can address Stephen Diehl's criticism of Haskell in production:

In practice any non-trivial business logic module can very easily have 100+ lines just of imports, and frankly it gets tiring. One common way of abstracting this is by rolling a custom prelude using module reexports.

texastoland commented Feb 21, 2016

Where do I read to understand imports in PureScript? It seems like they're unqualified by default (whereas Elm's are qualified as pointed out by @tfausak), no longer provide syntax to use unspecified without warning, essentially pushing users to either qualify or specify, motivated to prevent the chance of future collision.

Clashing names is why I thought minor versions exist in semver. I don't see why code should be noisy to prevent a break resulting from a minor change any more than a major one. Where it's a concern I assume users don't import unspecified or limit updates to patch level.

If all those are correct I'm missing how to use future preludes without warnings and how PureScript can address Stephen Diehl's criticism of Haskell in production:

In practice any non-trivial business logic module can very easily have 100+ lines just of imports, and frankly it gets tiring. One common way of abstracting this is by rolling a custom prelude using module reexports.

@garyb

This comment has been minimized.

Show comment
Hide comment
@garyb

garyb Feb 21, 2016

Member

Minor changes aren't supposed to break code though, if you use a version like ^1.0.0 that means 1.1.0 is also acceptable. If that introduces a new member that clashes, then it becomes a breaking change.

The criticism is valid, but I'll take a predictably non-breaking build over something that is occasionally tiring. The custom project-specific prelude approach may well become viable if we make the compiler aware of packages in the future.

Member

garyb commented Feb 21, 2016

Minor changes aren't supposed to break code though, if you use a version like ^1.0.0 that means 1.1.0 is also acceptable. If that introduces a new member that clashes, then it becomes a breaking change.

The criticism is valid, but I'll take a predictably non-breaking build over something that is occasionally tiring. The custom project-specific prelude approach may well become viable if we make the compiler aware of packages in the future.

@texastoland

This comment has been minimized.

Show comment
Hide comment
@texastoland

texastoland Feb 21, 2016

If you want to restrict to patch level changes can't you also impose ~1.0.0? It's not just about tooling it's readability and evangelizing users. At the heart of language design I want not only simplicity and security but developer experience.

I do like Elm's imports (albeit bundling their prelude is less nice). By default everything is qualified, unqualified is specified, and unqualified unspecified imposes a little noise to opt into. But warning for every import Batteries (or import Batteries exposing (..) or whichever syntax) doesn't seem nice ☹️

texastoland commented Feb 21, 2016

If you want to restrict to patch level changes can't you also impose ~1.0.0? It's not just about tooling it's readability and evangelizing users. At the heart of language design I want not only simplicity and security but developer experience.

I do like Elm's imports (albeit bundling their prelude is less nice). By default everything is qualified, unqualified is specified, and unqualified unspecified imposes a little noise to opt into. But warning for every import Batteries (or import Batteries exposing (..) or whichever syntax) doesn't seem nice ☹️

@tfausak

This comment has been minimized.

Show comment
Hide comment
@tfausak

tfausak Feb 21, 2016

Contributor

If you want to restrict to patch level changes can't you also impose ~1.0.0?

Exactly. This problem is already solved by tooling. If you're doing an open import of some other library and your specified version range for that library allows additions, you're going to have a bad time. The fix is not to disallow open imports; it's to specify a better version range.

If the compiler is aware of packages and y'all address this problem that way, you're forcing every package to define its own prelude in order to build without errors. That would be very annoying, especially since many of them would end up looking like this:

module My.Prelude (module Export) where import Batteries as Export
Contributor

tfausak commented Feb 21, 2016

If you want to restrict to patch level changes can't you also impose ~1.0.0?

Exactly. This problem is already solved by tooling. If you're doing an open import of some other library and your specified version range for that library allows additions, you're going to have a bad time. The fix is not to disallow open imports; it's to specify a better version range.

If the compiler is aware of packages and y'all address this problem that way, you're forcing every package to define its own prelude in order to build without errors. That would be very annoying, especially since many of them would end up looking like this:

module My.Prelude (module Export) where import Batteries as Export
@texastoland

This comment has been minimized.

Show comment
Hide comment
@texastoland

texastoland Feb 21, 2016

I see @garyb's point especially for libraries. IIUC import Neon (..) was essentially replaced with a bare import and a warning. My discomfort arises because I read warnings as gentle errors. The tradeoff seems like a beginner writes import Neon and tackles the warning or else a working PSer learns to tread carefully using open imports and restrict opened packages to patch level changes.

My big ask is twofold: how to preserve the beginner's experience and 2) how to prevent reading code from devolving into scrolling past imports or weird formatting conventions when they import multiple lines of identifiers from a single package.

texastoland commented Feb 21, 2016

I see @garyb's point especially for libraries. IIUC import Neon (..) was essentially replaced with a bare import and a warning. My discomfort arises because I read warnings as gentle errors. The tradeoff seems like a beginner writes import Neon and tackles the warning or else a working PSer learns to tread carefully using open imports and restrict opened packages to patch level changes.

My big ask is twofold: how to preserve the beginner's experience and 2) how to prevent reading code from devolving into scrolling past imports or weird formatting conventions when they import multiple lines of identifiers from a single package.

@texastoland

This comment has been minimized.

Show comment
Hide comment
@texastoland

texastoland Feb 21, 2016

One more thought. When we have a package manager that enforces semver, could it also impose patch level granularity downstream? I.e. if I left Prelude open when I port Foldl, could the package manager require clients to adhere to the same minor version of Prelude? If that's an eventual solution I'd feel more comfortable.

texastoland commented Feb 21, 2016

One more thought. When we have a package manager that enforces semver, could it also impose patch level granularity downstream? I.e. if I left Prelude open when I port Foldl, could the package manager require clients to adhere to the same minor version of Prelude? If that's an eventual solution I'd feel more comfortable.

@paf31

This comment has been minimized.

Show comment
Hide comment
@paf31

paf31 Feb 22, 2016

Member

No, let's please keep the issues to a minimum. Let's choose a path and rename this one.

Sent from my iPhone

On Feb 22, 2016, at 7:47 AM, Texas Toland notifications@github.com wrote:

The single open import idea would solve this anyway

doesn't apply to import Prelude if it's alone
I'm ready to open an issue for it. That leaves my concern about changing the semantics of import X to import X as X and corresponding proposal. Worth issues for each?


Reply to this email directly or view it on GitHub.

Member

paf31 commented Feb 22, 2016

No, let's please keep the issues to a minimum. Let's choose a path and rename this one.

Sent from my iPhone

On Feb 22, 2016, at 7:47 AM, Texas Toland notifications@github.com wrote:

The single open import idea would solve this anyway

doesn't apply to import Prelude if it's alone
I'm ready to open an issue for it. That leaves my concern about changing the semantics of import X to import X as X and corresponding proposal. Worth issues for each?


Reply to this email directly or view it on GitHub.

@texastoland

This comment has been minimized.

Show comment
Hide comment
@texastoland

texastoland Feb 22, 2016

@paf31 How do you feel about single open import? @garyb suggested it and @hdgarrood is on board. @garyb also suggested changing the meaning of import X (warning) to import X as X (no warning and akin to Elm). I offered the following clarifications:

  • Names in parens: unqualified or not (like Elm's exposing). Not in parens: qualified only.
  • import Neon (..): unqualified + unspecified (like 0.8.0).
  • import Neon as N: status quo.

E.g. import Neon as N (Add) would expose Add, N.Add, and N.Subtract, but not Neon.Add.

texastoland commented Feb 22, 2016

@paf31 How do you feel about single open import? @garyb suggested it and @hdgarrood is on board. @garyb also suggested changing the meaning of import X (warning) to import X as X (no warning and akin to Elm). I offered the following clarifications:

  • Names in parens: unqualified or not (like Elm's exposing). Not in parens: qualified only.
  • import Neon (..): unqualified + unspecified (like 0.8.0).
  • import Neon as N: status quo.

E.g. import Neon as N (Add) would expose Add, N.Add, and N.Subtract, but not Neon.Add.

@garyb

This comment has been minimized.

Show comment
Hide comment
@garyb

garyb Feb 22, 2016

Member

import X as X is unnecessary actually, as you can already reference X.y without requiring an import at all. At the time I suggested it we had discussed dropping that ability (for performance reasons) but we didn't end up needing to do that in the end.

Member

garyb commented Feb 22, 2016

import X as X is unnecessary actually, as you can already reference X.y without requiring an import at all. At the time I suggested it we had discussed dropping that ability (for performance reasons) but we didn't end up needing to do that in the end.

@texastoland

This comment has been minimized.

Show comment
Hide comment
@texastoland

texastoland Feb 22, 2016

@garyb Oh my bad I thought I had tried that. But if the way I want to use X, is qualified as X, is import X as X the only way to do so without a warning? Regardless your suggestion might be less surprising.

texastoland commented Feb 22, 2016

@garyb Oh my bad I thought I had tried that. But if the way I want to use X, is qualified as X, is import X as X the only way to do so without a warning? Regardless your suggestion might be less surprising.

@garyb

This comment has been minimized.

Show comment
Hide comment
@garyb

garyb Feb 22, 2016

Member

Yeah, I think so.

Member

garyb commented Feb 22, 2016

Yeah, I think so.

@texastoland

This comment has been minimized.

Show comment
Hide comment
@texastoland

texastoland Feb 22, 2016

I prefer your suggestion with import Prelude (..) being explicit, willing to tackle it DIY (especially since half the work is reverting a commit), but not passionate if y'all dislike it.

texastoland commented Feb 22, 2016

I prefer your suggestion with import Prelude (..) being explicit, willing to tackle it DIY (especially since half the work is reverting a commit), but not passionate if y'all dislike it.

@garyb

This comment has been minimized.

Show comment
Hide comment
@garyb

garyb Feb 22, 2016

Member

Oh, I also meant to respond to your suggestion about

import Neon as N (Add)

Although this would be super useful to have with it working the way you outlined, I worry that it would be confusing given that we already have:

import Neon (Add) as N

Which means something else, and is actually useful. It's required for cases where you import more than one module into N, as that suffers from the open-modules problem where conflicts can arise if the imports are unspecified. And also is useful for some re-export tricks, where you import some values from a module but then want to re-export different values.

Member

garyb commented Feb 22, 2016

Oh, I also meant to respond to your suggestion about

import Neon as N (Add)

Although this would be super useful to have with it working the way you outlined, I worry that it would be confusing given that we already have:

import Neon (Add) as N

Which means something else, and is actually useful. It's required for cases where you import more than one module into N, as that suffers from the open-modules problem where conflicts can arise if the imports are unspecified. And also is useful for some re-export tricks, where you import some values from a module but then want to re-export different values.

@paf31

This comment has been minimized.

Show comment
Hide comment
@paf31

paf31 Feb 22, 2016

Member

We also need to address the issue that two separate packages can define modules with the same name eventually, so I think we have to do something like this anyway.

What about allowing filename qualified imports, rather than package qualified? The compiler is aware of files, but not packages.

So, under the single open import suggestion, names in a closed import would shadow names in an open import without warnings? I'm not sure if I like that or not, since I think we already have enough ways to import something without beginners having to learn another rule, but it seems like a decent compromise. What if you have multiple open imports? Would they be shadowed too?

Member

paf31 commented Feb 22, 2016

We also need to address the issue that two separate packages can define modules with the same name eventually, so I think we have to do something like this anyway.

What about allowing filename qualified imports, rather than package qualified? The compiler is aware of files, but not packages.

So, under the single open import suggestion, names in a closed import would shadow names in an open import without warnings? I'm not sure if I like that or not, since I think we already have enough ways to import something without beginners having to learn another rule, but it seems like a decent compromise. What if you have multiple open imports? Would they be shadowed too?

@texastoland

This comment has been minimized.

Show comment
Hide comment
@texastoland

texastoland Feb 22, 2016

I worry that it would be confusing given that we already have:

@garyb I didn't consider that I'll noodle on it today. Does import Neon as N (Add) mean anything at present?

What if you have multiple open imports? Would they be shadowed too?

@paf31 Not according to @garyb:

If two open imports are used then go back to warning about all implicit imports.

texastoland commented Feb 22, 2016

I worry that it would be confusing given that we already have:

@garyb I didn't consider that I'll noodle on it today. Does import Neon as N (Add) mean anything at present?

What if you have multiple open imports? Would they be shadowed too?

@paf31 Not according to @garyb:

If two open imports are used then go back to warning about all implicit imports.

@garyb

This comment has been minimized.

Show comment
Hide comment
@garyb

garyb Feb 22, 2016

Member

So, under the single open import suggestion, names in a closed import would shadow names in an open import without warnings?

No, it would warn if that occurred but it wouldn't error, which will happen currently.

Member

garyb commented Feb 22, 2016

So, under the single open import suggestion, names in a closed import would shadow names in an open import without warnings?

No, it would warn if that occurred but it wouldn't error, which will happen currently.

@garyb

This comment has been minimized.

Show comment
Hide comment
@garyb

garyb Feb 22, 2016

Member

Oh sorry, if they're just shadowed they won't error currently, but that's the main thing I propose to change to make this single-open-import thing fix "The Problem". If x is in scope and used from both an implicit and explicit import the implicit version will be warned about, whereas currently conflicting members become an error regardless of how they arrived in the module.

Member

garyb commented Feb 22, 2016

Oh sorry, if they're just shadowed they won't error currently, but that's the main thing I propose to change to make this single-open-import thing fix "The Problem". If x is in scope and used from both an implicit and explicit import the implicit version will be warned about, whereas currently conflicting members become an error regardless of how they arrived in the module.

@paf31

This comment has been minimized.

Show comment
Hide comment
@paf31

paf31 Feb 22, 2016

Member

Seems like a reasonable solution. What are the downsides? It possibly makes more work for tooling, but that is a solvable problem. Can we think of any ways in which a dependency change could lead to breakage, which wouldn't be a warning?

Member

paf31 commented Feb 22, 2016

Seems like a reasonable solution. What are the downsides? It possibly makes more work for tooling, but that is a solvable problem. Can we think of any ways in which a dependency change could lead to breakage, which wouldn't be a warning?

@garyb

This comment has been minimized.

Show comment
Hide comment
@garyb

garyb Feb 22, 2016

Member

The downsides are it means having two modes of behaviour for the warnings depending on whether there are >1 open imports, and I mean that from both a compiler code and user point of view, but I don't think it will be too bad. Perhaps we can include something in the warning message when there are multiple open imports, making it clear that they're occuring because there is more than one open import.

I'm pretty sure it's safe, as the reason we imposed this change was to stop accidental conflicts occur when modules are extended, and since this is only allowed when there is one open module, if we auto-resolve the conflict to the explicit reference we're saved from that case.

Member

garyb commented Feb 22, 2016

The downsides are it means having two modes of behaviour for the warnings depending on whether there are >1 open imports, and I mean that from both a compiler code and user point of view, but I don't think it will be too bad. Perhaps we can include something in the warning message when there are multiple open imports, making it clear that they're occuring because there is more than one open import.

I'm pretty sure it's safe, as the reason we imposed this change was to stop accidental conflicts occur when modules are extended, and since this is only allowed when there is one open module, if we auto-resolve the conflict to the explicit reference we're saved from that case.

@paf31

This comment has been minimized.

Show comment
Hide comment
@paf31

paf31 Feb 22, 2016

Member

👍 Ok sounds good, I'll rename the issue.

Member

paf31 commented Feb 22, 2016

👍 Ok sounds good, I'll rename the issue.

@paf31 paf31 changed the title from How can I reexport without warnings? to Permit a single open import without a warning Feb 22, 2016

@texastoland

This comment has been minimized.

Show comment
Hide comment
@texastoland

texastoland Feb 22, 2016

@garyb Revised import proposal:

  • import X exposes X.f and X.A
  • import X (A) exposes X.f, X.A, and A
  • import X (..) exposes X.f, X.A, A, and f
  • import X (..) hiding (f) exposes X.f, X.A, and A (presumably?)
  • import X as Y exposes Y.f and Y.A
  • import X (A) as Y exposes Y.A (right?)
  • import X (..) as Y exposes Y.f and Y.A
  • import X as Y (A) is a syntax error

Furthermore cases with (..) warn.

texastoland commented Feb 22, 2016

@garyb Revised import proposal:

  • import X exposes X.f and X.A
  • import X (A) exposes X.f, X.A, and A
  • import X (..) exposes X.f, X.A, A, and f
  • import X (..) hiding (f) exposes X.f, X.A, and A (presumably?)
  • import X as Y exposes Y.f and Y.A
  • import X (A) as Y exposes Y.A (right?)
  • import X (..) as Y exposes Y.f and Y.A
  • import X as Y (A) is a syntax error

Furthermore cases with (..) warn.

@paf31

This comment has been minimized.

Show comment
Hide comment
@paf31

paf31 Feb 22, 2016

Member

Wait, we're not bringing (..) back, are we? I hope not.

Member

paf31 commented Feb 22, 2016

Wait, we're not bringing (..) back, are we? I hope not.

@texastoland

This comment has been minimized.

Show comment
Hide comment
@texastoland

texastoland Feb 22, 2016

If the default behavior of import X produces a warning it's a poor experience whereas (..) is strictly opt-in. Furthermore you must currently import X as X to use X as an alias without warning which looks counterintuitive.

texastoland commented Feb 22, 2016

If the default behavior of import X produces a warning it's a poor experience whereas (..) is strictly opt-in. Furthermore you must currently import X as X to use X as an alias without warning which looks counterintuitive.

@hdgarrood

This comment has been minimized.

Show comment
Hide comment
@hdgarrood

hdgarrood Feb 22, 2016

Contributor

I'm inclined to agree with @AppShipIt in that we should make the default/easiest path also be the safest/recommended one, but perhaps we should see if the single open import approach works before considering changing how import works.

Contributor

hdgarrood commented Feb 22, 2016

I'm inclined to agree with @AppShipIt in that we should make the default/easiest path also be the safest/recommended one, but perhaps we should see if the single open import approach works before considering changing how import works.

@texastoland

This comment has been minimized.

Show comment
Hide comment
@texastoland

texastoland Feb 22, 2016

@hdgarrood I'm fine with that especially since it will break things. Discussed offline it additionally provides some missing symmetry:

  • import X always introduces an identifier X
  • (foo) additionally introduces X.foo as foo
  • unless (foo) as Y clarifies foo as Y.foo
  • (..) is strictly necessary to open an import

And to clarify the breaking changes are:

  • import X becomes import X (..)
  • import X as X becomes import X
  • import X as X becomes redundant (just like it looks)

And Prelude isn't special case either: import Prelude (..).

texastoland commented Feb 22, 2016

@hdgarrood I'm fine with that especially since it will break things. Discussed offline it additionally provides some missing symmetry:

  • import X always introduces an identifier X
  • (foo) additionally introduces X.foo as foo
  • unless (foo) as Y clarifies foo as Y.foo
  • (..) is strictly necessary to open an import

And to clarify the breaking changes are:

  • import X becomes import X (..)
  • import X as X becomes import X
  • import X as X becomes redundant (just like it looks)

And Prelude isn't special case either: import Prelude (..).

@spicydonuts

This comment has been minimized.

Show comment
Hide comment
@spicydonuts

spicydonuts Feb 22, 2016

I like the single open import approach because most files want way more from one import than others (Prelude in most cases, but maybe React.DOM for view modules). It provides the most brevity while still being safe. It also never introduces the case where you're reading code and aren't sure where a function was imported from.

Do locally defined functions always override open module imports? That's the only edge case I can think of where updating dependencies would break things, but you've probably covered that already.

spicydonuts commented Feb 22, 2016

I like the single open import approach because most files want way more from one import than others (Prelude in most cases, but maybe React.DOM for view modules). It provides the most brevity while still being safe. It also never introduces the case where you're reading code and aren't sure where a function was imported from.

Do locally defined functions always override open module imports? That's the only edge case I can think of where updating dependencies would break things, but you've probably covered that already.

@texastoland

This comment has been minimized.

Show comment
Hide comment
@texastoland

texastoland Feb 22, 2016

Do locally defined functions always override open module imports?

@spicydonuts Any local identifier would shadow an imported clash with a warning.

texastoland commented Feb 22, 2016

Do locally defined functions always override open module imports?

@spicydonuts Any local identifier would shadow an imported clash with a warning.

@spicydonuts

This comment has been minimized.

Show comment
Hide comment
@spicydonuts

spicydonuts commented Feb 22, 2016

👍

@garyb

This comment has been minimized.

Show comment
Hide comment
@garyb

garyb Feb 22, 2016

Member

I don't know why I didn't think of this approach sooner 😉

Member

garyb commented Feb 22, 2016

I don't know why I didn't think of this approach sooner 😉

@texastoland

This comment has been minimized.

Show comment
Hide comment
@texastoland

texastoland commented Feb 22, 2016

@garyb Genius 🔮

@texastoland

This comment has been minimized.

Show comment
Hide comment
@texastoland

texastoland Feb 28, 2016

Should I move the import one to a new issue?

texastoland commented Feb 28, 2016

Should I move the import one to a new issue?

@garyb

This comment has been minimized.

Show comment
Hide comment
@garyb

garyb Feb 28, 2016

Member

Yes please. Assuming we do go ahead with it, it will be solved separately to the open import change anyway.

I'm going to have a look at making this change today too. 💃

Member

garyb commented Feb 28, 2016

Yes please. Assuming we do go ahead with it, it will be solved separately to the open import change anyway.

I'm going to have a look at making this change today too. 💃

@texastoland

This comment has been minimized.

Show comment
Hide comment
@texastoland

texastoland commented Feb 28, 2016

🙌

@mcfilib

This comment has been minimized.

Show comment
Hide comment
@mcfilib

mcfilib Apr 13, 2016

Is there any rationale anywhere I can read regarding removal of (..) and open imports more generally?

Currently going through the monotonous task of adding explicit imports and wondering whether, since purescript-psa (and presumably psc) can tell me what imports I should explicitly include, there should be a tool that can automatically add explicit imports to a project because I can assure you that doing it with my own hands is absolutely no fun at all!

mcfilib commented Apr 13, 2016

Is there any rationale anywhere I can read regarding removal of (..) and open imports more generally?

Currently going through the monotonous task of adding explicit imports and wondering whether, since purescript-psa (and presumably psc) can tell me what imports I should explicitly include, there should be a tool that can automatically add explicit imports to a project because I can assure you that doing it with my own hands is absolutely no fun at all!

@spicydonuts

This comment has been minimized.

Show comment
Hide comment
@spicydonuts

spicydonuts Apr 13, 2016

@filib psc-ide (now distributed with psc) has a number of editor plugins (emacs, vim, atom) and they support auto-adding explicit imports. They all support applying a suggestion to an import line (such as making an implicit import explicit) and the newest versions (emacs and atom I believe) support automatically adding imports for you when you first use a function.

spicydonuts commented Apr 13, 2016

@filib psc-ide (now distributed with psc) has a number of editor plugins (emacs, vim, atom) and they support auto-adding explicit imports. They all support applying a suggestion to an import line (such as making an implicit import explicit) and the newest versions (emacs and atom I believe) support automatically adding imports for you when you first use a function.

@nwolverson

This comment has been minimized.

Show comment
Hide comment
@nwolverson

nwolverson Apr 13, 2016

Contributor

There can certainly be such a tool (to take existing imports and fix them to qualified versions), this functionality is already in IDE plugins (at least it is in the Atom/VS Code ones), and as this is fundamentally from compiler output, there could be a command line tool to generate/apply a diff. The main issue for me there is the poor source position info from the compiler, but with the workarounds already in psa, it should be good enough to output a diff which can be checked.

Contributor

nwolverson commented Apr 13, 2016

There can certainly be such a tool (to take existing imports and fix them to qualified versions), this functionality is already in IDE plugins (at least it is in the Atom/VS Code ones), and as this is fundamentally from compiler output, there could be a command line tool to generate/apply a diff. The main issue for me there is the poor source position info from the compiler, but with the workarounds already in psa, it should be good enough to output a diff which can be checked.

@garyb

This comment has been minimized.

Show comment
Hide comment
@garyb

garyb Apr 13, 2016

Member

@filib Warning about open imports ensures a module cannot be broken by conflicts coming into scope if new members are added to a module that it imports. This generally isn't a problem within a single library or project, but we've had situations in the past where adding a member to one library broke libraries that depended on it.

And yes, there has been discussion about tooling to help with this, but I don't think there is anything for applying the warning suggestions automatically yet.

Member

garyb commented Apr 13, 2016

@filib Warning about open imports ensures a module cannot be broken by conflicts coming into scope if new members are added to a module that it imports. This generally isn't a problem within a single library or project, but we've had situations in the past where adding a member to one library broke libraries that depended on it.

And yes, there has been discussion about tooling to help with this, but I don't think there is anything for applying the warning suggestions automatically yet.

@texastoland

This comment has been minimized.

Show comment
Hide comment
@texastoland

texastoland Apr 13, 2016

removal of (..)

Returning in #1901

and open imports more generally

But will continue to warn for multiple.

monotonous task of adding explicit imports

As alluded above got implemented in #1967.

texastoland commented Apr 13, 2016

removal of (..)

Returning in #1901

and open imports more generally

But will continue to warn for multiple.

monotonous task of adding explicit imports

As alluded above got implemented in #1967.

@mcfilib

This comment has been minimized.

Show comment
Hide comment
@mcfilib

mcfilib Apr 13, 2016

Whoah! Thanks for such quick replies all!

@spicydonuts that would certainly help for additive changes. I tried giving psc-ide a shot with emacs and atom today but was unable to get either of them to work as they are documented. It's unclear as to why this was the case but I'll look into it later.

@nwolverson I would love if such a tool existed.

@AppShipIt @garyb thanks for the context.

mcfilib commented Apr 13, 2016

Whoah! Thanks for such quick replies all!

@spicydonuts that would certainly help for additive changes. I tried giving psc-ide a shot with emacs and atom today but was unable to get either of them to work as they are documented. It's unclear as to why this was the case but I'll look into it later.

@nwolverson I would love if such a tool existed.

@AppShipIt @garyb thanks for the context.

@kRITZCREEK

This comment has been minimized.

Show comment
Hide comment
@kRITZCREEK

kRITZCREEK Apr 13, 2016

Member

@filib If you could drop into IRC #purescript I could help setting psc-ide up for either Emacs or Atom.

Member

kRITZCREEK commented Apr 13, 2016

@filib If you could drop into IRC #purescript I could help setting psc-ide up for either Emacs or Atom.

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