Skip to content

Conversation

quickfur
Copy link
Member

@quickfur quickfur commented Dec 2, 2014

Workaround for: https://issues.dlang.org/show_bug.cgi?id=13808

The problem is that scoped imports pulls the imported symbols into the current scope, so if the import is in the body of a struct, the imported symbols will now hijack UFCS on the struct.

@joakim-noah
Copy link
Contributor

When you're repeating the same import inside unit tests so many times, it seems like using one version(unittest) to import it for all the unit tests in the module might be best, particularly when it's a selective import of just one symbol.

@dnadlinger
Copy link
Contributor

@joakim-noah: Sure, in theory that would be nice, but right now version (unittest) imports are the best way to break Phobos for all users in a way that is not detectable by the auto tester (the issue being that non-unittest code can accidentally depend on unittest-only imports).

@quickfur
Copy link
Member Author

quickfur commented Dec 2, 2014

Yeah, the problem is that unittests don't reside in a separate scope from the main code, so version(unittest) is actually polluting the main module scope. In normal code, this isn't a big problem because when you compile without -unittest any code that wrongly depends on a version(unittest) block will fail to compile, but in Phobos, you won't catch this error because the dependency can be from a template function, but without -unittest none of the templates are actually instantiated. So the breakage will be unnoticed until user code breaks, which is very bad.

@DmitryOlshansky
Copy link
Member

LGTM

@joakim-noah
Copy link
Contributor

Wouldn't a better way to fix this be to fix dmd so that symbols from a version(unittest) block are only available inside unittest{} blocks in the same module, as most would presume is already the case?

@quickfur
Copy link
Member Author

quickfur commented Dec 4, 2014

That would be ideal, but I don't know how long that will take. In the meantime, we have a regression in Phobos that needs to be fixed, as it's already breaking user code.

@Hackerpilot
Copy link
Contributor

@dnadlinger
Copy link
Contributor

Auto-merge toggled on

dnadlinger added a commit that referenced this pull request Dec 4, 2014
Issue 13808: Don't import std.format in any public scope.
@dnadlinger dnadlinger merged commit 6473aef into dlang:master Dec 4, 2014
@dnadlinger
Copy link
Contributor

Yeah, this at least un-breaks Phobos for now.

@quickfur quickfur deleted the issue13808 branch December 4, 2014 22:33
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.

5 participants