[TODO] Nim now allows modules with same name in a package #8614

timotheecour · 2018-08-11T10:11:29Z

No language I know except Nim has this restriction; lifting it just makes sense

Nim now allows modules with same name in a package, eg:
foo/bar.nim and bar.nim

this avoids having to use previous hacks like foo/foo_bar.nim that were there to avoid module name clashes; these were not DRY.

this enables to have tests/stdlib/ospaths.nim instead of tests/stdlib/tospaths.nim for a test of ospaths (EDIT as done for 1 test in this PR)
likewise, it allows things like:

json/serialization
bson/serialization

redis/types
mongodb/types

etc which are more natural than: redis/redis_types

if a user wants to import 2 modules with same basename in 1 module, he can just write:

import json/serialization as jserialization
import bson/serialization

links

NOTE

suppose we have:
/foo/bar/jesterRepo/util/baz.nim
with /foo/bar/jesterRepo/jester.nimble
it generates:
jester.util as package name EDIT
(ie using getPackageName which finds the first dir which contains a nimble file)

EDIT The following may be a bit surprising but are result of current placement of files in Nim:

stdlib.std.sha1
stdlib.pure.collections.intsets
compiler.compiler.prefixmatches # because compiler.nimble is under Nim/compiler.nimble instead of Nim/compiler/compiler.nimble => to be fixed in PR https://github.com/nim-lang/Nim/pull/8636

EDIT it generates package parents hierachically, exactly as follows:
module ospaths -> package stdlib.pure (note: fully qualified) => package stdlib
EDIT files like these will be generated in nimcache:

stdlib.pure.bitops.c
stdlib.pure.math.c
stdlib.system.c
timn.bugs.exp_module_nonuniq.t01.c

EDIT example of things it generates during compilation (with --hint[CC]=on):

CC: stdlib.pure.collections.rtarrays
CC: stdlib.std.sha1
CC: compiler.compiler.prefixmatches
CC: stdlib.pure.concurrency.cpuinfo
CC: stdlib.impure.nre.private.util
CC: stdlib.impure.nre
CC: stdlib.deprecated.pure.asyncio
CC: stdlib.system

TODO

EDIT add tests => tests/modules/tnotuniquename1.nim
EDIT not yet sure how to handle this 1 particular line: registerCallback c, "stdlib.*.staticWalkDir", proc (a: VmArgs) {.nimcall.} = ; @Araq ?

TODO (future PR)

move Nim/compiler.nimble to Nim/compiler/compiler.nimble => PR [TODO] [PENDING #8635] renamed: compiler.nimble -> compiler/compiler.nimble #8636
rename stdlib.nimble to std.nimble ? (would make more sense with import std/random) => PR [TODO] [pending #8636] rename stdlib.nimble to std.nimble for consistency #8637

depends on:

[trivial] tester.nim add more diagnostics #8631

previous versions of this PR

a previous version of this PR was using a simpler scheme:
/foo/bar/jester/util/baz.nim was mapped to @foo@bar@jester@util@ as package name ; this caused issues, eg with registerCallback and other tests, and made packages depend on location of nimble packages on filesystem, which is not desirable

then I made changes to use @jester@util@ instead of @foo@bar@jester@util@ (ie using getPackageName to find package root)

then I made changes to use jester.util instead of @jester@util@ which looks more natural

alehander92 · 2018-08-11T10:16:26Z

It would be cool if this limitation is lifted, I also find that it just leads to hacky workarounds

Araq · 2018-08-12T20:10:32Z

FYI this was actually on my TODO list too for the upcoming release.

euantorano · 2018-08-13T11:35:14Z

compiler/packagehandling.nim

@@ -18,7 +18,9 @@ iterator myParentDirs(p: string): string =
 proc resetPackageCache*(conf: ConfigRef) =
  conf.packageCache = newPackageCache()

-proc getPackageName*(conf: ConfigRef; path: string): string =
+when false:
+ proc getPackageName*(conf: ConfigRef; path: string): string =


Indenting here looks wrong, it should be 2 spaces where this appears to just be 1...

In fact, why not just remove this code instead of disabling it?

euantorano

Great, I've been hit by this limitation a couple of times now and would definitely welcome this change!

Clyybber · 2018-08-13T19:05:25Z

@Araq Is that TODO list public?

dom96 · 2018-08-13T20:17:45Z

Minor point, by why @?

Araq · 2018-08-13T21:05:22Z

@Clyybber With this feature implemented and my araq-misc branch merged, it'll be empty. (The other items are all on github.)

Araq · 2018-08-13T21:05:50Z

Minor point, by why @?

That won't stay, it's not allowed on Windows anyway.

…d of root of filesystem

… works on tests

timotheecour · 2018-08-14T22:05:05Z

@Araq @dom96

Minor point, by why @?
That won't stay, it's not allowed on Windows anyway.

Actually @ is allowed for filenames on windows, see https://stackoverflow.com/a/31976060/1426932 as well as green tests in #8629 which was a previous version of this PR using @ instead (I also tried to create foo@baron windows command prompt and it works)

However, I agree @ was not the best choice and now used . in this PR which fits better with rest of Nim.

PTAL (top-level PR msg reflects latest PR status)

dom96 · 2018-08-14T22:10:01Z

Why don't you just use _?

timotheecour · 2018-08-14T22:20:40Z

_ is bad because it creates ambiguities when folder or file contains _, which happens a lot, both in Nim repo and in other nimble repos, eg:
./lib/impure/db_mysql.nim
generates, with this PR, the following fully qualified name:
stdlib.impure.db_mysql

which is unambiguous (unless a Nim source dir/file contains an inner "." which we can disallow for dirs (it's already disallowed for nim files, and may even already be disallowed for Nim source dirs)

with your suggestion it would generate:
stdlib_impure_db_mysql
and would clash with following file, if it existed:
./lib/impure/db/mysql.nim
which maps to same string

(other eg: nim-ranges/ranges/ptr_arith.nim and ./doc/nimcache/manual/manual_snippet_119.nim)

. is just cleaner and fits better with rest of language which uses . for scope (eg ospaths.mysymbol)

timotheecour · 2018-08-15T06:59:39Z

@Araq @dom96 PTAL: tests are green

Araq · 2018-08-15T09:24:20Z

tests/modules/import_styles.nim

+import std/sha1 as sha1_2
+# TODO: Error: cannot open file: stdlib/std/sha1; inconsistent with the fact we
+# generate `stdlib.std.sha1`
+# import stdlib/std/sha1 as sha1_3


Why would import stdlib/std/sha1 be valid? It shouldn't be valid.

Araq

This is hard to follow and I don't understand why it's necessary. For the frontend you can just disable this check. For the backend you can replicate the Path structure or detect the part of the path required for disambiguation. Or you hash the full path and append it to the filename. These ideas should all lead to simpler code that doesn't touch so many parts of the compiler.

Araq · 2018-08-15T09:24:54Z

tests/newconfig/tfoo.nims

@@ -12,7 +12,7 @@ import ospaths
 warning("uninit", off)
 hint("processing", off)
 #--verbosity:2
-patchFile("stdlib", "math", "mymath")
+patchFile("stdlib.pure", "math", "mymath")


Get of this .pure requirement.

Araq · 2018-08-15T09:25:30Z

tests/testament/tester.nim

@@ -229,12 +229,20 @@ proc cmpMsgs(r: var TResults, expected, given: TSpec, test: TTest, target: TTarg
    r.addResult(test, target, expected.msg, given.msg, reSuccess)
    inc(r.passed)

+import compiler/options


Oh come on, the tester does not import the compiler.

Araq · 2018-08-15T09:26:23Z

tests/testament/tester.nim

@@ -262,6 +270,7 @@ proc codegenCheck(test: TTest, target: TTarget, spec: TSpec, expectedMsg: var st
    echo getCurrentExceptionMsg()
  except IOError:
    given.err = reCodeNotFound
+    echo getCurrentExceptionMsg()


There is nothing to report here, reCodeNotFound is sufficient.

this is introduced in this dependent PR #8631 and discussed there

Araq · 2018-08-15T09:26:58Z

lib/system/nimscript.nim

@@ -92,7 +92,7 @@ proc patchFile*(package, filename, replacement: string) =
  ##
  ## .. code-block:: nim
  ##
-  ##   patchFile("stdlib", "asyncdispatch", "patches/replacement")
+  ##   patchFile("stdlib.pure", "asyncdispatch", "patches/replacement")


The .pure part is an implementation detail and should not be exposed.

Araq · 2018-08-15T09:27:47Z

compiler/vmops.nim

@@ -16,19 +16,19 @@ from math import sqrt, ln, log10, log2, exp, round, arccos, arcsin,
 from os import getEnv, existsEnv, dirExists, fileExists, putEnv, walkDir

 template mathop(op) {.dirty.} =
-  registerCallback(c, "stdlib.math." & astToStr(op), `op Wrapper`)
+  registerCallback(c, "stdlib.pure.math." & astToStr(op), `op Wrapper`)


These should all not be changed, other tools also rely on registerCallback.

Araq · 2018-08-15T09:49:25Z

compiler/packagehandling.nim

@@ -12,38 +12,53 @@ iterator myParentDirs(p: string): string =
  var current = p
  while true:
    current = current.parentDir
-    if current.len == 0: break
+    # IMPROVE:parentDir is buggy, "foo.nim".parentDir should be ".", not ""


What? It should be .? No.

Araq · 2018-08-15T09:50:03Z

compiler/packagehandling.nim

-  var parents = 0
+  var dirs:seq[string] = @[]
+  var pkg = ""
+  var path_root = ""


The compiler and all of Nim's stdlib use camelCase.

GULPF · 2018-08-19T15:02:18Z

I think the docgen must be changed as well, it currently puts every HTML file in the same directory which will cause conflicts if there are duplicate module names.

dom96 · 2018-08-19T16:16:04Z

I think the docgen must be changed as well, it currently puts every HTML file in the same directory which will cause conflicts if there are duplicate module names.

I disagree. Worst case is the user will have to write -o:my_module2.html.

GULPF · 2018-08-19T16:35:36Z

I disagree. Worst case is the user will have to write -o:my_module2.html.

Using -o with a different file name means that module links that are generated by the docgen wont work. These are the links I'm referring to. Besides, writing the nim doc commands for individual modules isn't really feasibly in large projects (like Nims stdlib).

Clyybber · 2018-08-28T13:59:27Z

@timotheecour @Araq Will we get this merged before 0.19? If the comments are addressed at least?

Araq · 2018-08-28T14:58:13Z

Unlikely, sorry. v0.19 is overdue and bugs like #7854 take up all resources. :-)

Araq · 2018-12-21T17:15:40Z

As I said.

This is hard to follow and I don't understand why it's necessary. For the frontend you can just disable this check. For the backend you can replicate the Path structure or detect the part of the path required for disambiguation. Or you hash the full path and append it to the filename. These ideas should all lead to simpler code that doesn't touch so many parts of the compiler.

timotheecour closed this Aug 11, 2018

timotheecour reopened this Aug 11, 2018

euantorano reviewed Aug 13, 2018

View reviewed changes

timotheecour mentioned this pull request Aug 14, 2018

[DO NOT REVIEW] [ experiment #8614] using "@" for fully qualified names: stdlib@pure.ospaths #8629

Closed

timotheecour force-pushed the exp_module_nonuniq branch 2 times, most recently from 50b1867 to d2b9e8e Compare August 14, 2018 10:55

timotheecour mentioned this pull request Aug 14, 2018

[DO NOT REVIEW] [ experiment #8614] using "." for fully qualified names: stdlib.pure.ospaths #8635

Closed

timotheecour force-pushed the exp_module_nonuniq branch from d2b9e8e to 90db796 Compare August 14, 2018 21:46

timotheecour added 6 commits August 14, 2018 14:46

tester.nim add more diagnostics

17e485d

Nim now allows modules with same name in a package

7fc7f78

fix tests; make fqname be starting from root of nimble package instea…

d2969a9

…d of root of filesystem

fully qualified names as stdlib.pure.ospaths

720a4bd

add test import_styles.nim

821c72f

mv tests/stdlib/tospaths.nim tests/stdlib/ospaths.nim to make sure it…

b67bc2f

… works on tests

timotheecour force-pushed the exp_module_nonuniq branch from 90db796 to b67bc2f Compare August 14, 2018 21:46

Araq reviewed Aug 15, 2018

View reviewed changes

timotheecour mentioned this pull request Aug 28, 2018

[TODO] import ./[foo1,foo2] gives error; import ./bar/[foo1,foo2] works #8792

Closed

timotheecour mentioned this pull request Sep 4, 2018

[WONTFIX] [regression] nim doc foo/bar.nim should generate by default in htmldocs instead of foo/bar.html (makes it hard to gitignore otherwise) #8873

Closed

1 task

Araq closed this Dec 21, 2018

timotheecour changed the title ~~Nim now allows modules with same name in a package~~ [TODO] Nim now allows modules with same name in a package Dec 21, 2018

This was referenced Sep 6, 2019

Filenames ending with xxx8.nim #12130

Closed

fix #12130 ; improve naming scheme in fakePackageName #12149

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TODO] Nim now allows modules with same name in a package #8614

[TODO] Nim now allows modules with same name in a package #8614

timotheecour commented Aug 11, 2018 •

edited

Loading

alehander92 commented Aug 11, 2018

Araq commented Aug 12, 2018

euantorano Aug 13, 2018

dom96 Aug 13, 2018

timotheecour Aug 14, 2018

euantorano left a comment

Clyybber commented Aug 13, 2018

dom96 commented Aug 13, 2018

Araq commented Aug 13, 2018

Araq commented Aug 13, 2018

timotheecour commented Aug 14, 2018 •

edited

Loading

dom96 commented Aug 14, 2018

timotheecour commented Aug 14, 2018 •

edited

Loading

timotheecour commented Aug 15, 2018

Araq Aug 15, 2018

Araq left a comment

Araq Aug 15, 2018

Araq Aug 15, 2018

Araq Aug 15, 2018

timotheecour Aug 16, 2018

Araq Aug 15, 2018

Araq Aug 15, 2018

Araq Aug 15, 2018

Araq Aug 15, 2018

GULPF commented Aug 19, 2018 •

edited

Loading

dom96 commented Aug 19, 2018

GULPF commented Aug 19, 2018

Clyybber commented Aug 28, 2018

Araq commented Aug 28, 2018

Araq commented Dec 21, 2018

[TODO] Nim now allows modules with same name in a package #8614

[TODO] Nim now allows modules with same name in a package #8614

Conversation

timotheecour commented Aug 11, 2018 • edited Loading

links

NOTE

TODO

TODO (future PR)

depends on:

previous versions of this PR

alehander92 commented Aug 11, 2018

Araq commented Aug 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

euantorano left a comment

Choose a reason for hiding this comment

Clyybber commented Aug 13, 2018

dom96 commented Aug 13, 2018

Araq commented Aug 13, 2018

Araq commented Aug 13, 2018

timotheecour commented Aug 14, 2018 • edited Loading

dom96 commented Aug 14, 2018

timotheecour commented Aug 14, 2018 • edited Loading

timotheecour commented Aug 15, 2018

Choose a reason for hiding this comment

Araq left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GULPF commented Aug 19, 2018 • edited Loading

dom96 commented Aug 19, 2018

GULPF commented Aug 19, 2018

Clyybber commented Aug 28, 2018

Araq commented Aug 28, 2018

Araq commented Dec 21, 2018

timotheecour commented Aug 11, 2018 •

edited

Loading

timotheecour commented Aug 14, 2018 •

edited

Loading

timotheecour commented Aug 14, 2018 •

edited

Loading

GULPF commented Aug 19, 2018 •

edited

Loading