Skip to content
This repository has been archived by the owner on Oct 12, 2022. It is now read-only.

Add imported for inline imports #1756

Merged
merged 4 commits into from
Sep 15, 2021
Merged

Add imported for inline imports #1756

merged 4 commits into from
Sep 15, 2021

Conversation

andralex
Copy link
Member

@andralex andralex commented Feb 4, 2017

No description provided.

@JackStouffer
Copy link
Member

Is this intended to be a replacement or an addition to your DIP?

@andralex
Copy link
Member Author

andralex commented Feb 4, 2017

@JackStouffer looks like a solid candidate for replacement.

@JackStouffer
Copy link
Member

In this trivial example I am reading massive speedups. This looks promising.

But, the real test would converting a whole module from Phobos to this style, with no top level imports, and seeing the results.

import std.datetime;
import std.traits;

void func(T)(SysTime a, T value) if (isIntegral!T)
{
    import std.stdio : writeln;
    writeln(a, value);
}

void main() {}
$ time dmd test2.d
dmd test2.d  0.29s user 0.07s system 95% cpu 0.378 total

VS

template from(string moduleName)
{
    mixin("import from = " ~ moduleName ~ ";");
}

void func(T)(from!"std.datetime".SysTime a, T value) if (from!"std.traits".isIntegral!T)
{
    import std.stdio : writeln;
    writeln(a, value);
}

void main() {}
$ time dmd test2.d
dmd test2.d  0.07s user 0.02s system 83% cpu 0.114 total

@JackStouffer
Copy link
Member

I tried and failed to convert std.file to this format to get some performance numbers. Got some really odd and unhelpful error messages from the compiler when I removed std.range.primitives and std.traits from the top level.

@andralex
Copy link
Member Author

andralex commented Feb 5, 2017

@JackStouffer: yes, that was my experience as well, as I thoroughly documented in the initial discussion around DIP1005. It didn't produce much of an impression - it seems one must try it in order to understand the difficulties :).

@deadalnix
Copy link
Contributor

deadalnix commented Feb 5, 2017

If I may suggest a name change, I would use Module instead of from. Module!"foo.bar" will effectively be the foo.bar module so let's name it that way.

@jmdavis
Copy link
Member

jmdavis commented Feb 6, 2017

How would this work with UFCS? For some stuff, that doesn't necessarily matter much, but for something like the range primitives, it could matter a lot. For instance, if you have a function that accepts a range, and the template constraint uses the range primitives, the range primitives need to have been imported for the constraint to work with dynamic arrays. If the imports are just associated with the function as a whole like DIP 1005 does, then that's not a problem, but if you're using UFCS, then you need to import the symbol separately from using it, and this solution would appear to require that the import be associated with a symbol - or at least that's how all of the examples are using it.

Also, what happens with this if you're using multiple symbols from the same module inside of the parameters and/or template constraints? Do you have to use from with each symbol or only the first one? With DIP 1005, it's only once, so if this requires separate imports for each, that's a lot more verbose.

@atilaneves
Copy link
Contributor

@deadalnix I don't know, I think I like from better.

@MartinNowak
Copy link
Member

MartinNowak commented Feb 13, 2017

  • FWIW this achieves the same as lazy static import + FQN (Issue 13255 – static imports should be done lazily), except that from!"std.datetime" avoids the module-level declaration
  • seems like this doesn't (yet) drag in the imported ModuleInfo (doesn't add the import to ModuleInfo.importedModules), thus missing to run constructors of the imported module
  • from may not be the most prominent variable name, but it's definitely in use. Shouldn't collide much with this though.

@dnadlinger
Copy link
Member

dnadlinger commented Feb 13, 2017

+1 for Module. It actually documents the function, rather than just providing a cute syntax for a single use case. Even one of the examples in the documentation itself looks wrong:

with (from!"std.datetime")

Just observe how much better this reads:

with (Module!"std.datetime")

While cute-looking solutions have a habit of stroking one's ego, perpetuating the idea of programming as magic is precisely not what well-designed language artefacts are supposed to do.

@andralex
Copy link
Member Author

One matter is that the invocation solves to a module alias, not a type name, so by convention it needs to start with a lowecase letter, not uppercase. (I fail to grok the argument with cuteness, egos being stroked, and magic.)

@andralex
Copy link
Member Author

@klickverbot with will be rarely used because it cannot occur at top level and sheer import inside scopes is simpler and just as good if not better. So the prevalent use case would be in function signatures. So we'd be mainly looking at:

void popFrontN(R)(R range, size_t n) if (from!"std.range.primitives").isInputRange!R);

or

void popFrontN(R)(R range, size_t n) if (Module!"std.range.primitives").isInputRange!R);

etc. Both seem plausible (the naming convention is worth considering though).

@dnadlinger
Copy link
Member

dnadlinger commented Feb 14, 2017

One matter is that the invocation solves to a module alias, not a type name, so by convention it needs to start with a lowecase letter, not uppercase.

Depends on your convention. ;) I would assert that modules, just like ambiguous generic templates should be referred to as in uppercase. I suppose it depends on where you put the default – value-like or type-like.

Both seem plausible

Both do seem plausible. I'm just pointing out that the artefact admits more uses than just the <name>!"foo.bar".member syntax. For example, one might imagine use in template meta-programming code where string mixins are currently used to cobble together import statements. And if somebody does come up with another clever use in the future, a more descriptive name like Module would certainly be a better fit – even though from might be a tad shorter for the current use case and reads out in a suggestive way (what I was referring to as "cute" earlier).

@andralex
Copy link
Member Author

Depends on your convention.

Phobos uses this_convention for module names, see e.g. https://dlang.org/dstyle.html as well as phobos itself. It stands to reason that a construct yielding a module alias follows the same vein, just the same as a construct yielding a type uses the convention for types etc.

@WalterBright
Copy link
Member

The advantage to Module is it suggests that the string should be a module name, whereas from is considerably more generic and is not suggestive of what the string should be.

@jmdavis
Copy link
Member

jmdavis commented Feb 14, 2017

seems like this doesn't (yet) drag in the imported ModuleInfo (doesn't add the import to ModuleInfo.importedModules), thus missing to run constructors of the imported module

As long as this is the case, it seems like it would be a very bad idea to promote an idiom like this.

@WalterBright
Copy link
Member

seems like this doesn't (yet) drag in the imported ModuleInfo (doesn't add the import to ModuleInfo.importedModules), thus missing to run constructors of the imported module

Hmm, interesting. This also applies to local imports where the local import is inside a template, and the template is not instantiated by the module that defines the template.

However, the import should be added to the list of the module that instantiates it. Unfortunately, it does not. https://issues.dlang.org/show_bug.cgi?id=17181

@MartinNowak
Copy link
Member

Has anyone considered leaving away imports for fully qualified names?
We could likely teach the compiler to lazily static import std.range.primitives when it can't resolve std.range.primitives.InputRange.

@andralex
Copy link
Member Author

@MartinNowak Problem is it's unclear whether a.b.c.d refers to a fully qualified name or a lookup starting from the current scope. One possibility would be to enhance leading . to cause an attempt at an import if .a.b.c.d cannot be resolved. However, this leads to the second problem: what module should be imported? a, a.b, or a.b.c?

@dnadlinger
Copy link
Member

dnadlinger commented Feb 15, 2017

Would "non-locality" (e.g. having a preceding (static) import instead of using from/Module) be an issue even if it wouldn't come at any performance difference (which it shouldn't)? I certainly like that the fact that we can do this in a library artefact, but to be honest I'm still not sure whether there is an issue to solve in the first place.

@andralex: If we want to go for an auto-import feature, it seems like local scope disambiguation itself wouldn't be the biggest issue. We could simply anchor the automatic feature to the root of the lookup tree (i.e. "one above module-level imports"), with the same shadowing properties as today. The second problem – disambiguating between [a module].[b.c symbol] and [a.b module].[c symbol – seems to be the harder one to solve in a universal fashion.

(Edit for readability.)

@timotheecour
Copy link
Contributor

timotheecour commented Feb 15, 2017 via email

@MartinNowak
Copy link
Member

– disambiguating between [a module].[b.c symbol] and [a.b module].[c symbol – seems to be the harder one to solve in a universal fashion.

Maybe just try from longest to shortest module, taking the first match?
Collisions should be rare (also b/c covention for module and type names are different) and are an issue in the package.
This doesn't easily integrate with the rest of the lookup, e.g. a.b.T.create.ufcs, so indeed some separator, like a.b::T would be better to resolve ambiguity.

It seems the more important question is how important static imports in function/template parameters are. Modularity was named as a prime motivation.
Just wondering how much we'd be willing to do for this.
Making module level static imports lazily shouldn't be too complex and requires no language change either, same for selective imports.
https://issues.dlang.org/show_bug.cgi?id=13255

We could have a look at those after the current lookup changes are transitioned and the planned import refactorings are through (late H1, early H2).

@jmdavis
Copy link
Member

jmdavis commented Feb 22, 2017

Making module level static imports lazily shouldn't be too complex and requires no language change either, same for selective imports.

I would very much like to see lazy imports happen. I'm already getting sick of using local and selective imports all over the place instead of just importing modules. It does have the advantage of making it clear where stuff comes from and minimizing which parts of the module see the imports, but it's just plain tedious and adds maintenance overhead. As I understand it, having fully lazy imports would largely make local and selective imports unnecessary from a performance perspective, and as such, I would probably stop doing much with selective and local imports, because they're just so tedious. And going with from or DIP 1005 would just add to that. I'm not necessarily opposed to them existing, but I don't think that they're worth using - especially if we have lazy imports. IMHO, D function signatures already have way too much in the way of attributes and whatnot marking them up to add imports into the mix, much as it does have the nice property of making it clear which imports that function uses.

@joakim-noah
Copy link
Contributor

joakim-noah commented Mar 15, 2017

@jmdavis, I agree that it's too tedious to use local, selective imports everywhere, but I think the endgame is a process where you just stick a bunch of non-selective, module-scope imports up top while you're writing your code, ie the way almost all D code is written now, and then when done, run a tool over it to properly scope all the imports and make them selective. Doing it by hand is never going to scale, we need a tool.

Building such an import tool, by integrating the ddmd frontend with libdparse, is now third in my D queue, with the top two about to be cleared away. Lazy imports may take away the performance need for such a tool, but better documentation is well worth doing it anyway.

@jmdavis
Copy link
Member

jmdavis commented Mar 16, 2017

@jmdavis, I agree that it's too tedious to use local, selective imports everywhere, but I think the endgame is a process where you just stick a bunch of non-selective, module-scope imports up top while you're writing your code, ie the way almost all D code is written now, and then when done, run a tool over it to properly scope all the imports and make them selective. Doing it by hand is never going to scale, we need a tool.

IMHO, if we expect a tool to be used in order for your code to follow best practices in D, then we've screwed up. I have no problem with someone writing a tool that they think will make their life easier, but as I understand it, choices were made with D's features in order to avoid the need for tooling beyond the compiler - particularly with regards to boilerplate code. And I, for one, don't want to have to be running extra tools on my code. It should be reasonable to just write it and have it follow best practices.

And if having fully lazy imports largely gets rid of the need for scoped and selective imports, I'm very much in favor of having fully lazy imports as the solution. scoped and selective imports result in far too much boilerplate code and IMHO are a maintenance problem. They do provide some value in terms of encapsulating dependencies, but I just don't think that they're worth that.

@joakim-noah
Copy link
Contributor

joakim-noah commented Mar 17, 2017

I disagree. There is a certain class of source transformations that should be automated: for example, I can just write my code formatted fairly sloppily and then run dfmt over it when I'm done, to tidy up. Insisting that everything be done by hand is a relic of a bygone era. I agree that scoped imports are more verbose, but in this case, that's the appeal.

@jmdavis
Copy link
Member

jmdavis commented Mar 17, 2017

I disagree. There is a certain class of source transformations that should be automated: for example, I can just write my code formatted fairly sloppily and then run dfmt over it when I'm done, to tidy up. Insisting that everything be done by hand is a relic of a bygone era.

I have no problem with someone running a tool to do source transformations if they want to. I have a problem with us deciding that something is best practice which basically requires that we run a tool. You should be able to reasonably write good, clean code that follows best practices without needing to run additional tools if you don't want to. And scoped and selective imports border on needing a tool because of how tedious they are to write and maintain.

I agree that scoped imports are more verbose, but in this case, that's the appeal.

I don't understand this. The verbosity and the fact that they require a fair bit of upkeep in comparison to simply importing the module you need at the top is precisely the problem. You end up with a lot of extra lines in your code and a decent amount of extra effort - particularly when selective imports are added into the mix. If anything, selective imports make it exponentially worse. And if we had fully lazy imports, then the performance benefits of using them would be negated, making it far more reasonable to just import modules at the top and be done with it.

@joakim-noah
Copy link
Contributor

joakim-noah commented Mar 17, 2017

You should be able to reasonably write good, clean code that follows best practices without needing to run additional tools if you don't want to.

You can, nobody is stopping you from perfectly formatting your code and scoping all your imports from the first line you write. However, you can also just write your D code formatted sloppily and with top-level imports and then have the dfmt and dscope tools do that tedious work for you.

The key here is that nobody is making D coders perfectly format their code (like python's indentation rules) and scope all their imports. It is up to you if you want the benefits those provide and we'll be able to say, "Here are some tools to make your life easier."

You end up with a lot of extra lines in your code and a decent amount of extra effort

The extra lines have a purpose, they document where exactly the symbols are coming from, which lazy imports do not. I believe that is worth it, perhaps you do not.

As for extra effort, there's none required other than running the tool, but you seem to be against that. I agree that scoped, selective imports will not be used much if we require everybody to insert them by hand, but I'm not against tools to do that for us, if we choose to scope our imports.

@jmdavis
Copy link
Member

jmdavis commented Mar 17, 2017

I agree that scoped, selective imports will not be used much if we require everybody to insert them by hand, but I'm not against tools to do that for us, if we choose to scope our imports.

I'm not against the tool existing either. The problem is that increasingly, it seems to be that it's considered best practice to use scoped and selective imports, which is a pain to do by hand. So, if you want to follow "best practice," then it starts looking like a tool is needed in order for that to be manageable, which runs contrary to the idea that you should not need a tool for boilerplate in D (much as anyone is free to use a tool if they want one).

One of the reasons that scoped and selective imports are pushed for the way that they are is because of the performance benefits, and if we had fully lazy imports like Walter has talked about doing, then that benefit of scoped and selective imports would be negated. Folks could still use them if they wanted, and they could even use a tool to manage them if they wanted to, but I think that there would be a much weaker argument that it was best practice to use them. And after trying to use them for a while and getting sick of all of the extra, tedious work required to maintain them, I would very much like to see us not need them and not be in a position where it's tooted as best practice and that you should change your code to use them if you're not. It seems to me that fully lazy imports solve the primary problem while requiring a lot less work on the part of D programmers in general. IMHO, it would be a huge win.

@andralex
Copy link
Member Author

andralex commented Aug 4, 2021

@andralex Is this good to go? If so, please add a runnable unittest.

I was thinking we need more time to decide which approach to choose.

@andralex
Copy link
Member Author

andralex commented Aug 5, 2021

However, this is only possible if the parent module has a package.d

That's unfortunate. Was hoping for a scheme that minimizes the files opened and imported - the moment we get to package.d, that's liable to fetch a bunch of stuff.

@ljmf00
Copy link
Member

ljmf00 commented Aug 5, 2021

However, this is only possible if the parent module has a package.d

That's unfortunate. Was hoping for a scheme that minimizes the files opened and imported - the moment we get to package.d, that's liable to fetch a bunch of stuff.

Yes, that may impact performance. __traits(compiles, { import std.stdio; }) still opens a file descriptor. This can be optimized by the compiler because it only needs to search for module existence not actually opening it. Maybe a new trait is needed to achieve better performance.

This logic is needed for ambiguities like package.d containing a symbol with the same name of a module.

@dlang-bot dlang-bot removed the stalled label Aug 27, 2021
@dlang-bot
Copy link
Contributor

Thanks for your pull request, @andralex!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + druntime#1756"

@andralex andralex changed the title Add import_ for inline imports Add imported for inline imports Aug 27, 2021
@dlang-bot
Copy link
Contributor

Thanks for your pull request, @andralex!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + druntime#1756"

@ljmf00
Copy link
Member

ljmf00 commented Aug 27, 2021

Doesn't this get further discussion? 👀

I was thinking we need more time to decide which approach to choose.

@pbackus
Copy link
Contributor

pbackus commented Aug 27, 2021

@ljmf00 Given that the opDispatch version still needs the string-based version as a fallback in case of ambiguity, I do not think any opportunity is lost by merging the string-based version now and deferring the decision on the opDispatch version.

@ljmf00
Copy link
Member

ljmf00 commented Aug 27, 2021

@ljmf00 Given that the opDispatch version still needs the string-based version as a fallback in case of ambiguity, I do not think any opportunity is lost by merging the string-based version now and deferring the decision on the opDispatch version.

Fair point 👍

@RazvanN7
Copy link
Contributor

@andralex Please rebase this to fix the tester issues.

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @andralex!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + druntime#1756"

@dlang-bot dlang-bot merged commit 1d3f851 into dlang:master Sep 15, 2021
@nordlow
Copy link
Contributor

nordlow commented Sep 26, 2021

Are we supposed to make use of this template in as many places (in druntime phobos) as possible now? If so, I wonder if that is gonna lead to a non-insignificant overhead in space when doing, for instance, make -f posix.mak unittest in phobos. FYI, @kinke.

@andralex
Copy link
Member Author

I'd say cautiously at first, with an eye on impact on build times and quality of generated documentation.

@kinke
Copy link
Contributor

kinke commented Sep 26, 2021

[I'm mostly worried about aesthetics - from that POV, I'd prefer not seeing this used at all. ;)]

@nordlow
Copy link
Contributor

nordlow commented Sep 26, 2021

Why (again) where the following non-templated-bloated alternatives turned down:

foo(T)(T x)
if (isIntegral!T)
with(import std.traits)

,

foo(T)(T x)
if ((import std.traits).isIntegral!T)

, and

foo(T)(T x)
if ((import std.traits : isIntegral)!T)

turned down?

To site @andralex, all of these would "simply remove a limitation in the language".

@pbackus
Copy link
Contributor

pbackus commented Sep 26, 2021

Trying a library solution now does not mean we cannot add a new language feature later, if the library solution proves inadequate.

@nordlow
Copy link
Contributor

nordlow commented Sep 26, 2021

Trying a library solution now does not mean we cannot add a new language feature later, if the library solution proves inadequate.

Indeed. Moreover, a semi-automatic search--and-replace of imported!... would be a breeze.

Btw, when you say inadequate what comes to your mind, @pbackus?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.