Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

import this/foo which resolves this to <current nimble root> #267

Open
timotheecour opened this issue Oct 17, 2020 · 42 comments
Open

import this/foo which resolves this to <current nimble root> #267

timotheecour opened this issue Oct 17, 2020 · 42 comments

Comments

@timotheecour
Copy link
Member

timotheecour commented Oct 17, 2020

proposal

turn this into a reserved keyword (like std and pkg) when resolving imports, and resolves to the enclosing nimble package root directory

example

file layout:

$ pwd
/pathto/bar # git repo for bar nimble package

$ find .
bar.nimble
src/bar/foo1.nim
src/bar/sub/foo2.nim
src/bar/sub/foo3.nim
# in src/bar/sub/foo2.nim
import this/foo1 # resolves to /pathto/bar/src/bar/foo1.nim
import this/sub/foo3 # resolves to /pathto/bar/src/bar/sub/foo3.nim

rationale

  • this avoids having to compute relative paths, in particular ones starting with .., which are common especially in projects with multiple directories:
# from pkg/arraymancer
# in src/arraymancer/nn/activation/sigmoid.nim
import  ../../autograd

# with RFC:
import  this/autograd

# external use matches internal use, with s/this/arraymancer/
import  arraymancer/autograd
  • One additional advantage is that external use matches internal use, with s/this/arraymancer/, as shown above.

  • it's immediately clear which imports refer to current nimble package vs external packages, unlike with relative paths that don't start with .. (and even those don't guarantee that)

note

  • we can add the rule for backward compatibility that if import this/foo would've previously resolved to ./this/foo.nim (ie a directory ./this exists), then it would resolve to that
@ghost
Copy link

ghost commented Oct 17, 2020

You can already refer to the root of a Nimble package from itself just by using the package name, or am I mistaken?

@timotheecour
Copy link
Member Author

timotheecour commented Oct 17, 2020

Import hygiene mandates using relative paths.

Using package name would could resolve to a different path depending on what's in your nimble path, and in fact is a frequent source of gotchas for nimble test. As another example, compiler nimble package might be installed via nimble install compiler, and this would interfere with some import compiler/parser inside compiler code (eg during a ./koch boot).

See also nim-lang/fusion#25 (comment) for yet another example (for fusion bundled in stdlib)

@disruptek
Copy link

We already have too many ways to express this, and this change muddies the waters even further. It remains an option for the future; perhaps when it's a problem that many users share?

@dom96
Copy link
Contributor

dom96 commented Oct 18, 2020

This would require Nim knowing even more about Nimble than it already does. I don't think this is the direction we want to go in.

@mratsim
Copy link
Collaborator

mratsim commented Oct 18, 2020

You can already refer to the root of a Nimble package from itself just by using the package name, or am I mistaken?

When debugging users' Arraymancer issues I've already naively copy pasted their "import arraymancer" and used the one installed from "nimble install" instead if the one from the package.

This would require Nim knowing even more about Nimble than it already does. I don't think this is the direction we want to go in.

I have mixed feelings. One thing for sure is that I don't like how Python package import works as they require manipulating PYTHON_PATH before using any git cloned repo.

@dom96
Copy link
Contributor

dom96 commented Oct 18, 2020

When debugging users' Arraymancer issues I've already naively copy pasted their "import arraymancer" and used the one installed from "nimble install" instead if the one from the package.

If this is a common problem for you (or anyone else) then you may prefer to disable Nim reading these packages by removing https://github.com/nim-lang/Nim/blob/devel/config/nim.cfg#L52

@timotheecour
Copy link
Member Author

you may prefer to disable Nim reading these packages by removing

I don't see how this can work in practice, unless I'm missing your point; it's an all or nothing. If arraymancer depends on other nimble packages (which it does), you'd have to specify the --path for all (recursive) dependencies. For one off testing in isolation you can do NIMBLE_DIR=$HOME/.nimble_fake nimble install foo but that's not what this RFC is about.

This would require Nim knowing even more about Nimble than it already does. I don't think this is the direction we want to go in.

nim already knows about all of this, and needs this information already, see compiler/packagehandling.nim (getNimbleFile, getRelativePathFromConfigPath, nimbleDir etc).

The fact is that currently import foo can means too many different things:
import foo can refer to:

  • an internal file in ./foo.nim
  • an internal file in ./bar/foo.nim if we pass --path:bar
  • an stdlib module (eg: strutils)
  • an external nimble package

there's no way to tell how import foo will resolve unless you know the full context: file system layout, nimble packages installedm --path variables, nim stdlib modules.

This is error prone (as is the other gotcha mentioned with nimble path overriding local path), and also not self documenting in terms of what imports are external vs internal.

This proposal solves all those issues:

  • self documents which imports are internal to current project
  • avoids having to computer relative paths (especially ones with ..)
  • makes internal vs external imports identical modulo replacing this by pkgname
  • avoids ambiguities and overloading import foo with so many different context dependent meanings

@timotheecour
Copy link
Member Author

timotheecour commented Oct 24, 2020

this keeps happening, see latest CI important_packages breakage c-blake/cligen#170

This RFC would fix this issue cleanly and avoid any possible ambiguities:

# before RFC
import cligen/prefetch # bad, caused https://github.com/c-blake/cligen/issues/170
import prefetch # bad for reasons I gave above, everything is in same scope (internal vs external);
  # plus requires computing relative paths eg subdir/prefetch or ../prefetch
  # and these would look different from an external package importing it

# with RFC
import this/prefetch # 0 ambiguity, and external imports would be identical modulo s/this/pkgname/

@c-blake
Copy link

c-blake commented Oct 24, 2020

I consider this an artifact of bad nimble guidance. If nimble did not noisly complain about packages putting all their top-level modules at the top-level of the dir and push additional src/ or cligen/ subdirs and instead the common practice became to do exact that then import cligen/foo would always work - that cligen/ would just have to be in the search path either implicitly as part of a hierarchy or explicitly vi a nim.cfg. bin/ and tests/ and all that can still be subdirs (if packages even have/want that - like 90% are pure libs/non-bin/). The main ".h"-like import stuff should just not be. This just seems like a far simpler organization..much like what the whole C/C++ world uses.

You would have to import cligen/cligen unless there were a "deeper"/"more expansive" search path that included that top-level cligen/. Then you would only need the cligen/ package qualifier to disambiguate any module besides cligen itself (protected-ish by the central package DB). Expansivity/optional dropping of package qualification is an orthogonal topic, though.

Anyway, I've said all this before. This is maybe just yet another case where unnecessary complexities cause trouble.

@timotheecour
Copy link
Member Author

I don't see how removing src prefix would solve any issue import cligen/foo would still pickup what's selected in nimble if called from nimble-repo/cligen/sub.nim.
And requiring src prefix is a good thing for isolation, otherwise everything becomes in import scope, even unrelated packages, auxiliary files etc, regardless if they were intended for distribution.

much like what the whole C/C++ world uses

not sure i agree with that statement

@c-blake
Copy link

c-blake commented Oct 24, 2020

I continue to be dumbfounded about this src/-scope conflation. The whole package is already "scoped" in its git checkout dir, easily arranged to be the same as the package name. Under my idea, to "be installed" is simply to have that dir in the path. Most deps are library deps. Why complicate life? (EDIT: also "src/" scopes exactly nothing. It's the same name in every package. "cligen/cligen/" might do scoping work, but "src/" only separates "tests/", "bin/", "docs/" and the like from source which could all just be at the top-level - or whatever.)

@disruptek
Copy link

Agree; there are better places to spend the complexity budget, like making sure that versions parse correctly or that requirements result in the proper versions being installed.

@timotheecour
Copy link
Member Author

timotheecour commented Oct 24, 2020

The whole package is already "scoped" in its git checkout dir, easily arranged to be the same as the package name.

plenty of packages are in a git repo not named identical to the package name, eg:
criterion.nim/criterion.nimble
zero-functional/zero_functional.nimble
nim-libbacktrace/libbacktrace.nimble
and you wouldn't want git clone https://github.com/yglukhov/jsbind jsbind_debug to require being imported with import jsbind_debug

so that can't work; and the reason why you'd want to allow the git repo not be named identical to the nimble package name are obvious, eg if you already have nim-foo to distinguish from an already existing foo in your github organization, etc.

so you sill have to duplicate the package dir, and without src it looks more confusing without src, eg:
cd ~/git_clones/
git clone https://github.com/c-blake/cligen.git
layout:
~/git_clones/cligen/src/cligen/foo.nim # best practice
~/git_clones/cligen/cligen/foo.nim # without src

"src/" scopes exactly nothing

it does.

The fact that you didn't follow best practice in cligen by prefixing via src means that everything in your cligen git repo is in import scope without even being prefixed by cligen, eg:

nimble develop cligen
# this compiles but shouldn't, and can clash with other nimble packages
echo "import examples/cols" | nim r -

I suggest you patch cligen to follow the src/ convention, because it prevents that: it'd use --path:~/git_clones/cligen/src instead of --path:~/git_clones/cligen

@c-blake
Copy link

c-blake commented Oct 24, 2020

so that can't work.

Sure it can. You just git clone URI pkgname. Bonus points for realizing this is all you need to do for almost all library-like dependencies in my idea. One of the very few things a package manager might add value in doing is cloning such into an appropriately named directory which is what nimp tries to do. (I'm unsure I got normalization exactly right..) Anyway, if that one git_clones is in your nim.cfg then everything should just work. Do debug/jsbind not jsbind_debug and manage your path. that sounds more organized anyway. Maybe just myNimPkgBranches/ before MyNimGit_clones/.

"src/" scopes exactly nothing

it does.

No it doesn't. We are arguing at cross purposes. You seem to have accepted nimble or nimble develop or whatever as a valid constraint for "best practices" to be contingent upon. I'm arguing against it independently as if we were in a world without nimble-induced complexity.

I really think you should step back and imagine such a world. There is a very simple manual solution for most packages in that world with no package manager at all. "src/" pushes people away from that world by requiring either file motion or symlinks at "install" time or having "src/" in your imports or expansive/deep paths which cause even more potential for collisions. I know empirically that the biggest problems here are actually from the Nim stdlib not itself using its own std/ qualification.

Were I to change cligen/ layout (I have no specific plan to), it would be back to the original git_clones/cligen/[every module here] { and bin/ tests/ etc. at the same level, but I don't think the package manager really even needs to know about that substructure at all }. As already mentioned, everything is fully package-scoped/in need of package qual if .../git_clones is all that's in path. This means "to install" is literally nothing more than cloning it into a properly named directory in your path which is actually all that almost all dependencies need..very low configuration..almost no work, easily automated, and I think fundamentally less confusing though confusion can be subjective. It also obviates the need for your this/module.

Your echo "" | nim r - works fine for me outside of all nimbleness. (For cols it's not great since cols wants a stdin, but the import works fine.) I don't know (or want to know anymore) what nimble develop does. This isn't even just theoretical. This is how I've personally been using Nim for over 5 years. Somehow, I run into fewer problems than nimble users seem to (and just for basic install functionality). I'm always told "raise an issue". My response is, "This shouldn't even be a nimble responsibility.".

Anyway, I've said about as much on this topic as I care to, including writing an entire almost fully functional program demonstrating the ideas (which I use literally every day just as nimp up to easily update a farm of library repos). It's far fewer lines of code than text I've written on various github arguments..Maybe even this very one here. All I ever hear back are weird circular arguments that more or less just assume problem's of nimble's own creation, and I am not the only one who thinks this. So, good luck on this, but I'm done responding unless you say something that makes sense not assuming nimble-anything.

@timotheecour
Copy link
Member Author

timotheecour commented Oct 25, 2020

Were I to change cligen/ layout (I have no specific plan to), it would be back to the original git_clones/cligen/[every module here] { and bin/ tests/ etc. at the same level
everything is fully package-scoped/in need of package qual if .../git_clones is all that's in path

your suggestion is to use --path:/patho/git_clones, although it sounds simpler on surface, it's an all-or nothing approach that is much less flexible than what nimble offers.

With your approach, everything cloned under git_clones is available for import, you can't be granular about it.
To make one package un-importable, you'd have to remove the git cloned folder, which has global effect.

With the existing approach (what nimble uses), you can simply select which packages are importable explicitly eg:

# without mypkg1, mypkg2
nim --path:/patho/git_clones/mypkg1/src --path:/patho/git_clones/mypkg2/src main1.nim
# without mypkg2
nim --path:/patho/git_clones/mypkg1/src main1.nim

furthermore you can have several clones of the same package and select (via --path) which one you want, without global effects on the filesystem, all living under the same root /patho/git_clones/.

And finally, src makes it easy for generating docs, just traverse src and nothing more. Without src, you'd need explicit exclusions per package.

I don't know (or want to know anymore) what nimble develop does.

it's not specific to nimble. The fact that you have no src and everything in toplevel (eg https://github.com/c-blake/cligen/blob/master/examples/cols.nim) means that you're exposing import examples/cols to everything that can import cligen

git clone https://github.com/c-blake/cligen /patho/git_clone/cligen
nim c --path:/patho/git_clone/cligen main.nim
# main.nim
import cligen # ok, works
import examples/cols # bug: this is exposed to everyone that can `import cligen`

I don't know how to explain this any better than what I already did but maybe @dom96 can?

@disruptek
Copy link

@timotheecour what exactly are you afraid of? That someone will manage to position a file near yours and then manipulate your source to import it? That your source will accidentally import a file that it shouldn't have access to? Making files more accessible in the project isn't a bug; it's a feature.

If you want to change Nimble, there's a repository for that. This repository is for changes to the compiler. From that perspective, this change is counter to the approach @genotrance invented and which @c-blake and I support; that the package managers can determine the environment whole-cloth, build tools can optionally use this input from the package manager to invoke the compiler, and the compiler can optionally use input from the build tools to compile software.

The unidirectional flow of information makes it much easier for each participant to reason about the environment, and narrowing each scope is similarly useful -- the package manager just manages packages, the build tool just builds the project, and the compiler just compiles code. Simple.

If you want several clones of the same package (why?) then you can easily do that by changing your package manager to clone the same package several times. But please don't hoist more complexity on literally every single future user of the language -- just add a directory to your file system and point to it with your pm, build tool, or compiler. Thanks.

@genotrance
Copy link

For starters, I'm not for this simply because life's just fine with import "."/foo and import ".."/bar. File paths are how Nim resolves imports anyway so using strings doesn't kill anyone. Maybe . and .. don't work without "" and that could do with some added sugar but this proposal doesn't suggest any other fundamental issues with relative paths. Expecting users to learn yet another thing isn't worth it.

Meanwhile, some people don't use nimble and I'm also for removing all nimble / pkg manager specific code from Nim eventually. We have --path and nims/cfg and that's all you need to tell Nim where to look.

Lastly, there can be 500 ways to do package management and Nim should and can already support that. If nimble does not work well enough and anyone has found a better way, feel free to innovate and succeed on those merits. With hindsight, from scratch solutions can conveniently avoid nimble's legacy but do nothing to solve its issues.

@dom96
Copy link
Contributor

dom96 commented Oct 25, 2020

I don't know how to explain this any better than what I already did but maybe @dom96 can?

I will just say that as far as I understand, and I've discussed this matter with @genotrance at length, the package directory structure enforcement made by Nimble will remain. I've even tried to push for a way to resolve it because of the amount of passionate push back from the likes of @c-blake, but we couldn't come up with a better alternative. The reason I'm saying this is because I feel like there is an implicit blame that rests on me when it comes to this feature and that is completely unfair.

@c-blake
Copy link

c-blake commented Oct 25, 2020

@dom96 - I meant/mean nothing personal or unfair, though my tone may become strident the more sure I am that I am right or the more off track counter arguments seem. I am indeed passionately against src/ (and unnecessary hierarchy/complexity in general) and think it should never have existed and I did complain the moment I was pushed into it by someone due to scary sounding nimble build|check errors. People keep responding with "namespace" or "scope isolation" responses which make no sense to me.

If you really care about qualification, then push import foo/foo and one shallow path. If you want deep paths, I've actually tested nimp against large swatch of the nimbleverse. There are very few bumps in that road. As mentioned, the stdlib itself should use std/ more. Otherwise, a stdlib import cpuinfo picks up laser cpuinfo, for example (only in "deep path" mode, not "surface path" import foo/foo mode. I don't personally mind shallow/surface mode. I think theory (what could be) may get in the way of practice (what is) here.

To me, every single time someone uses nimble init and it creates a src/ and they publish it, things become worse/more locked in. The vast majority of nimble packages (around 85% IIRC) are single module anyway getting exactly zero leverage out of being in a src/ directory. So, one can imagine all kinds of crazy scenarios, but designing for the crazy over the common to the detriment of the common is not great on the common user. The vast majority of packages are pure libraries needing literally no work because that's just how Nim libraries work (except for code generators like @genotrance's nimterop/toast). Python had all this slowness and really needed C shared libs in some src to build into a lib. Can nimble even build shared libraries? I don't recall ever seeing one. People I know hate the pip/virtualenv parts of Python and that is a selling point of Nim. I do see people maybe doing sub-git-repos to build C libraries to wrap in Nim, using whatever the build machinery is of the C package which I honestly also doubt is something nimble should know anything about. If the decision is that it will stay, then IMO that decision is one for continuing to make more of a "mess for the many for problems of the few (or the none)".

exposing examples/
exposing tests/
docs without exclusions

@timotheecour, to all of these I say, much like @disrpuptek, so what? I think I agree with both his statements and reasoning 100%, especially the information flow/separation of concerns. To follow up on complaints you may feel unaddressed, if client code imports something under examples/ or tests/ in it then either they deserve what they get OR they know exactly what they're doing in which case it might be fine. Some tests or examples may have some killer code you'd rather import than copy at some point and you may trust the package author (or be them!) to only add not delete them or screw with module names. Maybe a package author wants to test out some example or test either at the doc-level or import-level before moving it into a less risky-seeming place for their users. Maybe you want examples/ to have doc generation for discoverability and if you don't want docs in tests/ then don't use a ##. Just use single # or something. Guess what? Client code can always mis-import. import of things like "private" or "tests" or "examples" is obviously risky just from the text already. Ability to import examples/foo is just a non-problem in the first place with various possible good uses.

In general, this src/ thing aka file motion at install time (which @Araq also does not like) tries to solve a non-problem (bad import) by creating more rules (install dynamics) instead of just letting obvious interpretations stand. Organizing things "a level up" instead of down is also as global or local as you want based on how many paths you want. I'd expect most people would want one..three package namespaces - shared packages, entirely per a root package with all its deps and maybe a "myHacks" layer that could override shared packages, but these are just possibly convenient examples.

Super diverse repo/branch scenarios should not complicate the life of 99% user when there is a simple solution already using directories and search paths in the natural way (analogizing .nim files more to .h files) and does not block super-duper cases. Honestly, the more super duper a user you are, the more I would think you would want the package manager to do even less since it's more likely to get whatever it does do wrong/inconvenient.

For complex version scenarios, I could have one top-level dir switch latestTag/work_dirs and another head/work_dirs for everything and with that one switch right on the nim c command line change what I test client code against en masse. I don't have to pick one or the other, I can test both. For a single root package with really complex requirements I could just give it all its own working dirs. They could even all share the same .git storage if disk space is an issue. I think such scenarios will be rare and when compiles/when declared makes Nim a super adaptable language almost entirely steering clear of version hell. I only ever made big incompatible changes in cligen like once in almost 6 years with seq/string losing nil which I had been using a ton. Or maybe clCfg but that left most use cases alone. Importing multiple versions in one program also sounds like a nightmare. Let's just not do that. Such massive backward incompatible changes should be so rare and major-number-only that just having a cligen2 would be enough, IMO. (That will probably never happen with cligen, though, and as mentioned I think it will be much more rare with Nim packages than in other languages.)

In terms of this RFC more specifically, I agree with @genotrance about just using "." (possibly with a quotes requirement) is better than a new keyword "this/foo". It's probably what most people think "." should do already anyway. That's sort of how $PATH search in Unix works. C preprocessors have a similar distinction between #include "foo.h" (tight binding) and #include <foo.h> (search path) which relies on quoting with "" or <>.

I already think module import should allow stropping import <backtick>foo-bar<backtick> so that "module identifiers" have the same generality as regular Nim idents and at some point @Araq was persuaded. Combining that with a literal ./ with a tight-binding interpretation might make sense, as in maybe either "."/[one,two] or <backtick>.<backtick>/[one,two], maybe with distinct meanings. (Or maybe not..I am mostly just brainstorming here. Probably just an explicit dot or not either quoted or stropped {EDIT: or no quoting at all} is the right answer {EDIT: and having import ./foo mean "first search relative to the directory of the importing module" with no conceptual contact whatsoever with "package" ideas }).

@Araq
Copy link
Member

Araq commented Oct 26, 2020

The fact is that currently import foo can means too many different things:
import foo can refer to:
an internal file in ./foo.nim
an internal file in ./bar/foo.nim if we pass --path:bar
an stdlib module (eg: strutils)
an external nimble package

If it can be interpreted as "./foo.nim" that is the interpreation that is picked up. I try to avoid "--path" as far as reasonable. The fact that it's "import strutils" and not "import std / strutils" is a legacy and you can view these modules as keywords. New standard modules are always under "std", so it's a fixed set of keyword-like imports.

Having said that, I agree with @c-blake's and @disruptek's views. Nimble created a set of problems (and not only solutions) and we're getting a better Nimble thanks to @genotrance's superb work. As for "nimp" and "nimph", I don't mind package manager experiements, heck I did my own with "nawabs" (now dead) but we're well served with a single standard tool. Heretical thought: Rename "nimp" to "nimble" (version 2.0) and make it reasonably command line compatible. I know it's not realistic, but even such a radical solution would cause far fewer disruptions than multiple competing packager managers.

@Araq
Copy link
Member

Araq commented Oct 26, 2020

Also, to give Nimble some credit: The "namespacing" design predated our import path / [moduleA, moduleB] syntax and so a certain amount of syntactic sugar was necessary.

@Araq
Copy link
Member

Araq commented Oct 26, 2020

I already think module import should allow stropping import foo-bar so that "module identifiers" have the same generality as regular Nim idents and at some point @Araq was persuaded.

Confirmed, still seems like a good idea. :-)

@Araq
Copy link
Member

Araq commented Oct 26, 2020

Regardless of how all these things work or should work, what is the benefit of "this" over the existing and widely used "."?

Let's please close this RFC. The discussion is interesting but has been done elsewhere already.

@timotheecour
Copy link
Member Author

timotheecour commented Apr 23, 2021

@Araq I think we should seriously consider this RFC, it removes ambiguities; with this RFC:

  • users can tell whether an import is within same package or crosses pacakge boundaries
  • you get a CT error if a module is (missing/moved/absent with some compiler option) instead of accidentally resolving to an unintended module in your path/stdlib

in nim-lang/Nim#17426 (comment) you mention:

The correct way is import macros, inside std import other std modules via relative imports.

however this guideline doesn't work, nor should it IMO: for eg in lib/pure/strutils.nim if you change import std/enumutils to import enumutils, nim c --lib:lib compiler/nim will fail with: Error: cannot open file: enumutils

and writing import ../std/enumutils here would be even worse.

with this RFC, this would simply be:

import this/enumutils

with 0 possible chances of clashing with something you're not expecting, and this works from anywhere inside the same package (stdlib in this case), without having to compute relative paths (../std/enumutils), nor with any hacks.

to re-iterate:
if an external module would use import mypkg/bar/foo, internally you use import this/bar/foo; here are a few examples:

import this/os
import this/tables
import this/enumutils
import this/private/since # not this/since

@timotheecour timotheecour reopened this Apr 23, 2021
@Varriount
Copy link

users can tell whether an import is within same package or crosses pacakge boundaries

Won't they be able to tell this naturally? If they're importing using a relative import, it can be assumed that they're importing within the package. Otherwise, they're using an absolute import, which will indicate the same thing.

you get a CT error if a module is (missing/moved/absent with some compiler option) instead of accidentally resolving to an unintended module in your path/stdlib

How often does this actually happen? The compiler produces output on what modules are used, and if, by some rare chance, you do have a module with the same name, you'll get compile-time errors regardless.

@c-blake
Copy link

c-blake commented Apr 23, 2021

@timotheecour , besides @Varriount's fine questions, you still are not motivating why this and not .. However the stdlib happened to decide to locate files vs. doing its module search is not quite an argument as it seems to be trying to do some special namespacing tricks with std/ (e.g. pure/math can be imported as std/math). import PKGNAMEpkg is already an ugly workaround for self-imposed nimble design decisions. Avoiding a single backslash quote in a regex search and replace is also not a strong argument (as in replace "\./" vs "this/" - who cares?). It's also been raised by at least 3 people in this thread alone..who knows counting IRC/forum/etc., including @Araq's final pre-close comment. { EDIT finale -> final, though Araq's comments can be both final and a finale :-) }

@Araq
Copy link
Member

Araq commented Apr 23, 2021

without having to compute relative paths (../std/enumutils)

But relative paths are the best solution in my opinion -- clear, independent of the --path state, not machine specific. We should discourage --path and --path-like solutions and encourage relative paths. They work and are the simplest design that works.

Also... the "lib/pure" setup is a hack and standard modules should be in "std" anyway. However -- we still need a way to get os2.nim and json2.nim modules into the standard, somehow. Different topic.

@timotheecour
Copy link
Member Author

timotheecour commented Apr 23, 2021

Won't they be able to tell this naturally? If they're importing using a relative import, it can be assumed that they're importing within the package. Otherwise, they're using an absolute import, which will indicate the same thing.

it's not possible without having to do detective work, eg look at https://github.com/yglukhov/nimx/blob/master/nimx/view.nim

import typetraits, tables
import types, context, animation_runner, layout_vars
import property_visitor
import class_registry
import serializers
import kiwi
import notification_center

which of those are local vs external imports?
kiwi turns out to be external, serializers is internal, tables is stdlib, etc.

add to the mix the fact that --path can be provided, that stdlib has multiple ways to refer to the same module, the fact that nimble packages don't respect hygiene (multiple top-level scope), and you end up with something that's error prone and gets worse over time as more nimble packages are added.

you still are not motivating why this and not .

. doesn't imply relative import, import ./macros works outside of stdlib (and resolves to stdlib's macros). And even if that were changed, ./foo has the issues i mentioned in RFC rationale regarding having to compute relative paths like ../foo which is bad because it's harder and doesn't look like the way you'd import from outside that package.
Furthermore, I've seen people frown upon ./ prefix (and import ./[foo] doesn't work, but could)

there's no way to tell whether a path like import foo or import foo/bar is relative or external unless you clone the repo (at the same revision) and see whether bar or foo/bar resolve to some file in the same repo, ie it requires knowing the context (and even that is not enough, because of --path which can be in some config.nims/cfg file, or what nimble packages are installed etc)

import this/foo solves all those problems in a simple way, and brings back proper namespacing hygiene

We should discourage --path and --path-like solutions and encourage relative paths.

as mentioned above, there's no way to tell whether import foo is relative path or resovlves to external. And here nim-lang/Nim#17426 (comment) you've recommended import macros which is not a relative path from lib/std/genasts.nim to lib/core/macros.nim

@c-blake
Copy link

c-blake commented Apr 23, 2021

. doesn't imply relative import

But . (EDIT: or ./[]) could imply relative import. I'd bet most would be surprised that it does not, actually. It could be relative to the location of the importing module not the package, as in "fail if the import starts with . or .. and no local module is found" rather than "fail over to a path search".

I don't think package-anything should have to connect with this. The less we intertwine the compiler import system and the packaging system the better, IMO. Partly because the packaging system seems like a mess, but partly just on foundational separation of concerns terms.

I don't see cut & paste & translate imports from a defining package to a client package as a very common use case to optimize for (or in your words "looking the same"). If many-module-in-deep-folder-hierarchy package authors want that property, they can get it by importing with ..s all the way up to the package root and then back down. Then the client could just delete all the leading ..s and replace with the package name. Done. Users can bug them to import that way as more "transportable style", and fight it out..Worst case this will encourage more shallow hierarchies which are almost always easier to understand anyway.

@timotheecour
Copy link
Member Author

timotheecour commented Apr 23, 2021

But . (EDIT: or ./[]) could imply relative import. I'd bet most would be surprised that it does not, actually

I've suggested that before, see nim-lang/Nim#8608, but it was closed.

see also @Araq's remark in nim-lang/Nim#7250 (comment)

./foo is ugly and relative to the current module should have the highest priority. IMO.

IMO relative imports SHOULD start with ./ or ../, and this should become the prefered style; we can develop tooling to help in this migration, but that would require changing the above stance regarding ./foo being ugly (which i don't agree with, it's explicit)

EDIT:
note that import "."/tables also doesn't (currently) require tables to be a local module, this works (but shouldn't) outside of stdlib

@ee7
Copy link

ee7 commented Apr 23, 2021

Orthogonal questions:
Can we nudge users towards a better import style in another way? Perhaps via a compiler hint, or by adding it to nimpretty?

Taking the example from #267 (comment)

import typetraits, tables
import types, context, animation_runner, layout_vars
import property_visitor
import class_registry
import serializers
import kiwi
import notification_center

Is the below the most restrictive way to write it? Should we consider it the most readable/idiomatic?

import std/[typetraits, tables]
import pkg/kiwi
import "."/[types, context, animation_runner, layout_vars]
import "."/property_visitor
import "."/class_registry
import "."/serializers
import "."/notification_center

Somewhat similar to the UnusedImport warning, I suppose the compiler could produce a hint like

/tmp/foo.nim(1, 1) Hint: 'import typetraits, tables' could be: 'import std/[typetraits, tables]' [ImportStyle]

But maybe disabled by default, given that styleCheck hints are disabled by default.

I believe we can't add it to styleCheck directly because it wouldn't be backward-compatible with existing code that uses --styleCheck:error.

@Araq
Copy link
Member

Araq commented Apr 23, 2021

I still think it's ugly, but more importantly, what else should import x mean? It shouldn't be import $somepackage / x. I don't mind import ./subdir / x if that's what is required so that subdir cannot possibly be a Nimble package.

@timotheecour
Copy link
Member Author

timotheecour commented Apr 23, 2021

Is the below the most restrictive way to write it? Should we consider it the most readable/idiomatic?

import "."/serializers doesn't imply serializers is relative, nor does import ./serializers.
it's IMO a bug that should be fixed (try it: nim r --eval:'import "."/tables' works, but shouldn't)

so this gives (currently) a false sense of hygiene, but if this were fixed, i would agree with requiring/encouraging this new syntax (but with ./foo instead of "."/foo simpler is better)

what else should import x mean?

the problem is that it can mean different things depending on context (maybe x is a nimble package, eg regex: import regex resovles to external; or maybe it resolves to stdlib, or to a local path)

I don't mind import ./subdir / x if that's what is required so that subdir cannot possibly be a Nimble package.

that would be clear progress, but it's not enough. These should hold:

# this must resolve to a local import, ie `currentSourcePath.parentDir/foo.nim`:
import ./foo

# ditto
import ./bar/foo

# ditto
import ./[foo, bar]

# this is fine:
import std/tables
import pkg/regex
import pkg/regex/foo

this should give a deprecation warnings eventually (maybe after tooling is developped to help fix those automatically):

import foo # warning because unclear without context whether this would resolve as local or external
import foo/bar # ditto

as @yglukhov mentioned here: nim-lang/Nim#8608 (comment)

Well I consider the current import semantics to be flawed. Yes, adding pkg magic word allows more control over the user intention. However, there remains a flaw, that the users will use pkg magic only when they hit a problem, to workaround it, otherwise they will just import foo as long as it works [...]

@Araq
Copy link
Member

Araq commented Apr 23, 2021

Why not like this:

import std / tables # sure, what else.

import tables # deprecated, but allowed for old std modules.

import ast # local, relative import

import "." / ast # same as  'import ast' but discouraged because longer

import ".." / [ast] # local, relative import

import compiler / ast # local, relative import

import pkg / compiler / [ast] # 'pkg' means take --path and --nimblePath into account

This way only import std/ and import pkg/ are special cases. And only these depend on --lib or ``--path` state.

EDIT: I do not recommend the pkg practice, I was merely thinking aloud.

@c-blake
Copy link

c-blake commented Apr 23, 2021

Another unmentioned aspect in play here is at odds with the entire general notion of "precise import", but takes a bit of elaboration to explain. A client package (in nimble or not, just client code really) being able to replace just one module file from something it depends upon. I have a few things in cligen that I set up explicitly to be replaceable by client code and at least one user that likes that property (stuff like what config file parser to use). Are there other ways to do that? Sure. But replacing the whole file seemed simplest. For those things I would definitely not use an explicit enforced relative import. So there, there would be this dichotomy of import one way to allow override/import another to disallow.

It's not always easy to know when to disallow, though. So, you kind of want to lean toward allowing it almost all the time. E.g., someone might want to replace std/parsejson, say, with their own hacked copy that they know is compatible (or they are willing to take compatibility risks with/track stdlib changes). Maybe it's faster, smaller memory or whatever than std/parsejson. Json is just one example used by other things in the stdlib. std/tables is another or std/random with some 13x faster shuffle. They might even want std/json or std/marshal to pick up their better parsejson, no longer found under std/. But if std/json does an (enforced) import ./parsejson or even (std/parsejson) then this cannot happen.

So, if the whole stdlib (or something else beyond your control that has a lot of cross internal dependency) is doing precise, enforced relative imports everywhere then you need to put a clone of the whole thing, or at least all modules that transitively import anything you wanted to change into your source tree. Yuck. Hygiene in the small has now led to a mess in the large (not unlike nimble's src/ shenanigans which actually impact this use case, too, but that is a longer story.). And, honestly, that is a far more tricky computation than the ".." thing @timotheecour was complaining about.

You may object to this "surgical substitution" example as an ill-advised, obscure uncommon use case, but it was actually one of the things that made Symbolics Lisp Machines of the 1980s so popular among people I've spoken to who used them. The power of simple tweaks should not be underestimated, especially among developers whose tastes and priorities can differ dramatically. It's obviously a kind of "expert mode", and it can cut both ways, of course, by breaking things, but Nim is also a kind of a great power/great responsibility language in most of its philosophy.

I think of it a lot like defining some new variant of a proc in a local scope, just at the module level, with the "patch" facilitated by the language, not blocked, like overriding some stdlib hash function. So, I think some kind of it should be allowed in some way (syntax/compiler flag/etc.), and I think the current pkg/std "directions" are already off track on these terms. I don't have a specific proposal. I am just raising an unraised related concern that I don't think has been raised. I'm sure it's occurred to Araq who seems to just be trying to make everyone happy which may be hard/impossible { see developer tastes/priorities above :-) }.

TL;DR - getting a different module than what "looks obvious" can be a feature, not a bug, and the more intricate the web of dependencies, the more likely it can be a feature, actually, because that's when it's hardest to replace other ways. So, there are some real conflicting concerns here. Maybe the resolution is "screw the expert mode people wanting to hot patch/hack patch the stdlib/other stuff". Maybe that's already written in stone and just "assumed", but I didn't think it should go without mention. Or maybe I'm missing some logical step making such substitution easy in these New World Orders underway/being proposed. Apologies if so.

@c-blake
Copy link

c-blake commented Apr 23, 2021

Maybe a better TL;DR - proc names are (usually) unqualified not thisModule.procname and that has pros as well as cons. Merely possessing a "version tag" needn't change package/module name management so much. The closer we can keep all name management the same the better.

@ee7
Copy link

ee7 commented Apr 23, 2021

Why not like this:

Looks good to me. Especially if:

import ./tables       # is an error/warning if there is no local, relative `tables`
import "."/tables     # is an error/warning if there is no local, relative `tables`
import ./[foo, bar]   # no longer produces an error if `foo` and `bar` are local, relative imports
import "."/[foo, bar] # works like `./[foo, bar]`, which we could encourage instead

@c-blake Do you think the config.nims + patchFile approach is good enough?

Some related forum threads:

@c-blake
Copy link

c-blake commented Apr 23, 2021

Ok. I was just reading the docs for patchFile. (EDIT: I had assumed it altered the file, not the location of the file.) Sorry for commenting before doing that. Maybe it's an ok answer..but I don't know if it overrides things like std/ or pkg/. (EDIT: it should also be able to override these new "precise" local imports, too.)

@c-blake
Copy link

c-blake commented Apr 23, 2021

Well, I just tested it and patchFile("stdlib", "parsejson", "somethingsomething") continues to work if you hack the import ..., parsejson in stdlib json to import std/[.., parsejson]. So, that's good. Not sure about new "pkg" keywordy stuff, but there seems no test for that in the Nim repo (and the extant test does not test something else doing import std/math but gets mymath). Sorry again - I don't use patchFile because just putting a file in place has worked forever, but we're discussing things to make that go away. FWIW, aliasModule, substFile, replaceModule, switchFile, or even swapModule might be more suggestive names.

I still think package/module name management should be as close to regular module/symbol as possible and not interact much with the package manager, and I think we're off that road. As a strawman analogy, nimscript shenanigans to define a new local scope proc or var would be showstopper awful.

@Varriount
Copy link

Are the current import semantics documented anywhere?

@Varriount
Copy link

Why not like this:

import std / tables # sure, what else.

import tables # deprecated, but allowed for old std modules.

import ast # local, relative import

import "." / ast # same as  'import ast' but discouraged because longer

import ".." / [ast] # local, relative import

import compiler / ast # local, relative import

import pkg / compiler / [ast] # 'pkg' means take --path and --nimblePath into account

This way only import std/ and import pkg/ are special cases. And only these depend on --lib or ``--path` state.

@Araq So, this would mean that non-stdlib "external" modules (like say, NPeg) would be referenced through "pkg"?

@dom96
Copy link
Contributor

dom96 commented Apr 30, 2021

@Araq So, this would mean that non-stdlib "external" modules (like say, NPeg) would be referenced through "pkg"?

My understanding is "yes".

@Araq FWIW I think your idea is a pretty good one. One big problem is that it will break code though.

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

No branches or pull requests

9 participants