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

Revert "fix Issue 13855 - multi-module selective import statements" #7939

Merged
merged 1 commit into from
Feb 23, 2018

Conversation

wilzbach
Copy link
Member

@wilzbach wilzbach commented Feb 22, 2018

This reverts commit 0585416 (added in #6589)

On the NG this feature seems highly controversial and imho we regardless whether this makes it in 2.080 or not, imho it's clear that this feature needs more time (and a proper DIP).

See e.g. https://forum.dlang.org/post/[email protected] for the NG discussion.

Syntax ambiguity

(from @schveiguy)

import foo, baz;

Now I want to limit the import of foo to only have the symbol bar:

import foo : bar, baz;

Now, I've completely changed the meaning of the import of baz, without realizing it (and the compiler accepts it!).

Also comments like this one were mentioned:

Some of us think that it's a bad feature and have no intention of ever using it, though once it's in the language, we all have to worry about dealing with code that does use it.

Imho the wide disapproval of this shows that it shouldn't carelessly be pushed in 2.079, which would been it's more or less irrevertible.
Hence, I suggest to revert it for the upcoming 2.079, s.t. there's more time to think about this.

CC @JinShil @WalterBright @andralex @MartinNowak

@dlang-bot
Copy link
Contributor

dlang-bot commented Feb 22, 2018

Thanks for your pull request, @wilzbach!

Bugzilla references

Auto-close Bugzilla Severity Description
13855 enhancement Allow multiple selective imports from different modules in a single import statement

@aravindavk
Copy link

+1

Even Python's Official style checker discourages multiple imports in single line even though it is a valid syntax. https://www.python.org/dev/peps/pep-0008/#imports

@joakim-noah
Copy link
Contributor

I agree on this pull pushing this feature for later. I'm not against this feature in principle, but the syntax clearly needs work.

@wilzbach
Copy link
Member Author

CyberShadow/DAutoTest — Test failed

This is due to our new Spec tester :)
So it indeed enforces that the spec is always up-to-date.

-> dlang/dlang.org#2237

@schveiguy
Copy link
Member

FYI, I'm against the feature in general. I can't see why we need to complicate import statements -- you don't have to put your imports on separate lines:

import foo: bar; import baz : bop;
// vs import foo: bar, baz : bop;

I'd be against even adding different separators as has been suggested on the forums, though at least they wouldn't have ambiguities.

We'd be better off exploring more ideas like the "self-important" trick.

@andralex
Copy link
Member

I think we should keep the feature.

@schveiguy
Copy link
Member

@andralex:

module foo;

int bar();
int baz();
module baz;
...
module mainfile;

import foo : bar, baz;

Does this mean, import foo.bar and module baz, or foo.bar and foo.baz? Obviously, it has to mean the latter, but now it's ambiguous to read, because modules can be imported at the same time as selective imports.

I just can't see the benefit to this complication. You can put multiple styles of imports on one line, just separate them by ; import.

@schveiguy
Copy link
Member

schveiguy commented Feb 22, 2018

This is something we can't easily undo, and I think it came as a surprise in the about-to-be-released changelog list to many people. Regardless of whether it makes it into 2.080 (as it very well could), wouldn't it make sense to wait a month rather than deal with having to rip it out later?

@andralex
Copy link
Member

andralex commented Feb 22, 2018

@wilzbach the syntax import pkg : mod1 : sym1, mod2 : sym2; doesn't work, could you please amend the top comment.

@schveiguy The ambiguity claim is unfounded, seeing as any other viable semantics for the syntax would be just perverse.

@jmdavis
Copy link
Member

jmdavis commented Feb 22, 2018

I would point out that the comments in the Beta thread talking about this feature pretty much all seem to either want a different syntax or don't want the feature at all. Personally, I think that it's far too confusing and hard to read for it to be worth having. In the original PR, Walter seems to have thought that it harmed legibility as well.

@schveiguy
Copy link
Member

schveiguy commented Feb 22, 2018

any other viable semantics for the syntax would be just perverse

Strawman, wasn't saying I wanted an alternate syntax. The current syntax is not perverse, and it works just fine. Edit: I misread your statement, yes, it's correct that an alternative interpretation is not viable.

But imagining a scenario where someone wants to import baz module, and has an existing import statement like:

import std.stdio: writeln, foo : bar;

The natural thing is to tack it on the end. But that doesn't work. In other words, the expected thing isn't what is interpreted.

And the claim is not unfounded, since the only crime a module has to commit in order to be unimportable in one of these lines is that it's not in a top-level package. That should have nothing to do with its importability.

@schveiguy
Copy link
Member

Another scenario:

import foo, baz;

Now I want to limit the import of foo to only have the symbol bar:

import foo : bar, baz;

Now, I've completely changed the meaning of the import of baz, without realizing it (and the compiler accepts it!).

@schveiguy
Copy link
Member

And yes, I know this would happen today. But today it's not valid to import a whole module and selective imports in the same line. So the expectation that it should work isn't there.

@MetaLang
Copy link
Member

Wouldn't it be enough to require parentheses around the symbol list to disambiguate? e.g.:

import pkg.mod1 : sym1, mod2 : (sym2, sym3);

And then if the parens aren't there, it is assumed that sym3 is a separate module/package.

@schveiguy
Copy link
Member

schveiguy commented Feb 22, 2018

@MetaLang no, because import mod2 : sym2, sym3 works as a selective import today, we don't want to break code.

as Andrei said, it's not ambiguous. The expectation that I can just tack on another import though, will lead to confusion.

@jacob-carlborg
Copy link
Contributor

I think we should remove the feature.

@timotheecour
Copy link
Contributor

it prevents importing top level modules in these one liners (except in 1st position, or if they have selective imports). The fact we have such edge cases is a giant code smell. And is easy to get wrong (transforming meaning of imports if prepending other imports, as noted in other comments). It's simply broken.

@wilzbach
Copy link
Member Author

I agree on this pull pushing this feature for later. I'm not against this feature in principle, but the syntax clearly needs work.
...
Regardless of whether it makes it into 2.080 (as it very well could), wouldn't it make sense to wait a month rather than deal with having to rip it out later?

👍

Yes, the entire point of the revert was to avoid pushing a feature that might bite us in the neck later and FWIW such a controversial feature should have a DIP.

I paraphrase someone from the NG:

You force people to invest a whole lot of time and effort in writing a DIP for e.g. enum and function attributes and let the DIP and PR rot even though it's also "turtles all the way down" here and compared to the import syntax has only received positive feedback (The DIP is at +5, whereas import syntax was at +4, -4).

@d-random-contributor
Copy link

dlang/phobos#5948 was given as a reason for the feature. Do selective imports have performance impact? I thought most of performance comes from conversion of global imports to local imports. And most imports I see in that PR are single imports.

Selective imports tend to be big, so cramming them into one statement and then wrapping the statement into lines defeats the intention to reduce line count. I tend to tinker with local selected imports too, but this issue never occurred to me: local non-selective imports are fine too: they are already local, so don't affect much code.

This is a more readable syntax:

import pkg.mod1(sym1), mod2(sym2, sym3);

Though I don't think the feature pulls its weight.

@wilzbach
Copy link
Member Author

dlang/phobos#5948 was given as a reason for the feature. Do selective imports have performance impact?

Nope they don't improve the compilation time (at least as of now). Though a future DMD compiler could do imports fully lazily with selective or static imports.

I thought most of performance comes from conversion of global imports to local imports. And most imports I see in that PR are single imports

Yes and much more importantly avoiding crazy things at CTFE during the compilation time (e.g. parsing, and running semantic on the unicode tables is quite expensive at the moment - it takes about 0.1s + std.uni builds Tries on top of this at CTFE too).

@MartinNowak
Copy link
Member

Thanks for opening a PR @wilzbach. Let's see what we conclude in the forum.

Copy link
Contributor

@JinShil JinShil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO this needs broader support to make it into a release.

@joakim-noah
Copy link
Contributor

Nope they don't improve the compilation time (at least as of now). Though a future DMD compiler could do imports fully lazily with selective or static imports.

Actually, I think they do, as otherwise the compiler has to search for the symbol in every imported module and every other module they recursively publicly import, which could be a fair amount for the stdlib. That's probably why the symbol search function is marked with a comment that it should not be slowed down, while the search should be scoped considerably for a selective import. Maybe the increase in compilation speed is not always noticeable, but I think it should be there.

Copy link
Member

@MartinNowak MartinNowak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, deferring this until we have resolved a broader discussion about imports seems worthwhile.
We've already identified that there are 2 separate use-cases scripting/hacking and libraries/programming. Given that 2.079 also comes with std.experimental.scripting there is some indication that imports are sometimes in the way. Maybe we can find better solutions to address the different needs.

@MartinNowak
Copy link
Member

it prevents importing top level modules in these one liners (except in 1st position, or if they have selective imports). The fact we have such edge cases is a giant code smell. And is easy to get wrong (transforming meaning of imports if prepending other imports, as noted in other comments). It's simply broken.

Thanks @timotheecour, that makes a good argument.
Personally I always considered the fact that only the last module can have a selective import list a weird edge case, and top-level modules are rare. But it's certainly true that it's a hack on the weird existing grammar and could complicate tooling significantly.

@wilzbach
Copy link
Member Author

Rebased as the spec revert.

@d-random-contributor
Copy link

parsing, and running semantic on the unicode tables

Code there is pretty nice, if the two static ifs at the top level were replaced with version statement, it should be possible to collect all symbols without semantic analysis at all.

@wilzbach wilzbach deleted the revert-6589 branch March 13, 2018 17:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.