Split `std/times` #18378

konsumlamm · 2021-06-28T15:26:41Z

This PR splits off two modules from std/times, which are both reexported:

std/times/core contains core types and procs used by the other modules
std/times/durations contains the types and procs related to durations

I used the {.all.} pragma and importutils.privateAccess to be able to access internal definitions from the core and durations modules, that are needed by times, as I didn't want to create separate private modules that export everything and are then partly reexported.

The CI is currently failing, partly due to my use of std/importutils (cc @timotheecour), I'm investigating this. A part of the reason is that operators are redefined to not include overflow checks for the JS backend (since that's apparently broken), is that really necessary (shouldn't that either be handled in system or not at all)?

Add module `std/times/core` Add module `std/times/durations`

Remove `{.raises: [Defect].}`

timotheecour · 2021-06-28T16:36:41Z

as the CI logs say:

2021-06-28T16:08:28.9585520Z /Users/runner/work/1/s/lib/std/importutils.nim(23, 41) Warning: unknown magic 'PrivateAccess' might crash the compiler [UnknownMagic]

PrivateAccess is only available for nim >= 1.5.1 so this can't be used during bootstrap unless csources_v1 gets updated to 1.6

Other than that, nothing against splitting std/times, but IMO we should build Durations on top of int128 instead of what we have (object: seconds: int64, nanosecond: NanosecondRange), as it'll make the math simpler and more efficient, avoiding having to use base-1M operations everywhere, refs:

APIs should have typesafe self-documenting time units, eg proc sleep(t: Duration), allowing sleep(1.millisecond) etc RFCs#383 (comment)
type TimeUnit = distinct int128 timotheecour/Nim#762
the question is whether this can be done transparently or whether we need a new type

konsumlamm · 2021-06-28T16:44:46Z

PrivateAccess is only available for nim >= 1.5.1 so this can't be used during bootstrap unless csources_v1 gets updated to 1.6

Does that mean I shouldn't use PrivateAccess (which would make things a lot uglier imo) or wait for 1.6 with this PR?

konsumlamm · 2021-06-28T16:49:36Z

lib/pure/times.nim

-  # This is really bad, but overflow checks are broken badly for
-  # ints on the JS backend. See #6752.
-  {.push overflowChecks: off.}
-  proc `*`(a, b: int64): int64 =
-    system.`*`(a, b)
-  proc `*`(a, b: int): int =
-    system.`*`(a, b)
-  proc `+`(a, b: int64): int64 =
-    system.`+`(a, b)
-  proc `+`(a, b: int): int =
-    system.`+`(a, b)
-  proc `-`(a, b: int64): int64 =
-    system.`-`(a, b)
-  proc `-`(a, b: int): int =
-    system.`-`(a, b)
-  proc inc(a: var int, b: int) =
-    system.inc(a, b)
-  proc inc(a: var int64, b: int) =
-    system.inc(a, b)
-  {.pop.}
-


I decided to remove this for now, to make things easier. This still needs to be addressed somehow though, perhaps by disabling overflow checks for the JS backend in system?

this would be addressed by timotheecour#762 (int128 which can be implemented on top of jsBigInt on js backend)

timotheecour · 2021-06-28T18:01:42Z

lib/pure/times.nim

@@ -2067,7 +1673,7 @@ template formatValue*(result: var string; value: Time, specifier: string) =

 proc parse*(input: string, f: TimeFormat, zone: Timezone = local(),
    loc: DateTimeLocale = DefaultLocale): DateTime
-    {.raises: [TimeParseError, Defect].} =


see #16724 (comment)
this requires csources_v1 >= 1.4.0

Does that mean I should undo this change (also see #18378 (comment))?

unless you can figure out a way that works during bootstrap, I don't see another way.

you can test locally whether it works by building:

bin/nim_csources_a8a5241f9475099c823cfe1a5e0ca4022ac201ff c compiler/nim
bin/nim_csources_a8a5241f9475099c823cfe1a5e0ca4022ac201ff c koch

So I also can't use importutils.privateAccess/{.use.}?

you mean {.all.}, not {.use.} ? indeed. It can be used in code that's not used during compiler bootstrap or needs when defined(nimHasHintAll) or any recent such condsyms symbol if you can get away with custom implementation for the needed symbols during bootstrap

timotheecour · 2021-06-28T18:04:45Z

lib/std/times/durations.nim

+# Duration
+#
+
+const DurationZero* = Duration() ## \


is there any style guide still recommending ## \ ? if so they should be updated, it doesn't seem necessary (simpler = better)
as can be seen with:

nim doc -r main

const a1* = 10 ## \ ## ok1 ## ok2 const a2* = 10 ## ok1 ## ok2

My bad, I forgot to change that (times used this before, for some reason).

separate cleanup PR welcome; better to separate non-controversial PRs from more difficult ones so that the non-controversial one gets merged fast and doesn't complicate the other one

arnetheduck · 2021-06-28T18:39:52Z

While you're here: #17072

timotheecour · 2021-06-28T18:43:51Z

While you're here: #17072

yes, but it can be done independently of this PR (either before or after)

Araq · 2021-06-28T19:38:12Z

So I also can't use importutils.privateAccess/{.use.}?

No, you can't and I don't see why it's necessary. Use proper layering instead.

konsumlamm · 2021-06-28T19:50:28Z

So I also can't use importutils.privateAccess/{.use.}?

No, you can't and I don't see why it's necessary. Use proper layering instead.

The problem is that for example std/times needs access to the internal procs from std/times/durations. The only other way to do this (since afaik Nim as no "package-private" access modifier), that I can think of, is putting everything in some private module and then reexport part of that in std/times/durations, but that breaks the generated documentation, as it only shows a list of reexported symbols.

If there's another way to do this, please tell me (sorry if I'm missing something obvious).

timotheecour · 2021-06-28T20:06:57Z

but that breaks the generated documentation, as it only shows a list of reexported symbols.

this is a common problem with re-exports in similar cases, we could patch docgen to show docs for re-exported symbols (with an indication that it's a re-export); it's not critical though as nagivation in docs will still work so you can click the re-export and see its docs (even if it points to a private module). The important things to preserve are:

symbols should be transparently movable to another module via import+re-export (no change in semantics for client code)
docs are visible somehow in generated html (whether in re-exported module or private module), and no duplication happens in source code
once csources_v1 is upgraded to 1.6, this intermediate layer can disappear again (removing raises Defect only requires bootstrap >= 1.4)

The other point (regarding Duration as int128) is more tricky but important.

Araq · 2021-06-29T05:50:23Z

that I can think of, is putting everything in some private module and then reexport part of that in std/times/durations, but that breaks the generated documentation, as it only shows a list of reexported symbols.

That would be the proper way though. But yeah, considering that then the docs are more annoying to use, can't we leave times.nim alone instead please?

Araq · 2021-06-29T06:08:14Z

symbols should be transparently movable to another module via import+re-export (no change in semantics for client code)

IMHO re-exports are bad style and a last resort.

docs are visible somehow in generated html (whether in re-exported module or private module), and no duplication happens in source code

That mitigates the problems of re-exporting but avoiding re-exports is still the better solution.

once csources_v1 is upgraded to 1.6, this intermediate layer can disappear again (removing raises Defect only requires bootstrap >= 1.4)

There are no plans to update csources to 1.6 anytime soon btw, if we need to write new compiler in a compiler2 directory, so be it.

timotheecour · 2021-06-29T08:22:50Z

if we need to write new compiler in a compiler2 directory, so be it.

not sure how that would help with bootstrap issues (including for building koch which depends on many things, but also for building compiler/nim).

As for docs + re-export, it's a solvable problem that only requires a targeted (docgen) patch. Besides the solvable docs problem, I'm not sure what are objective downsides of re-export (eg it doesn't give ambiguous symbol error since it's the same symbol)

Araq · 2021-06-29T08:32:53Z

Re-exports are bad because they add access paths to a symbol: You type module.foo and the compiler expands it to moduleB.foo in an error message, it's great. Re-exports are also usually used because Nim doesn't attach procs to types, Java and C# don't have re-exports.

timotheecour · 2021-06-29T09:09:11Z

Re-exports are bad because [...]
Java and C# don't have re-exports.

... but D, python3, swift, rust, js, typescript, etc even C++ modules and C's #include, all support re-export, and for good reason, it allows changing implementation without affecting client code, and layering the API.

Error messages and docs only require targeted fixes, eg keeping the mapping in some Table[PSym, PSym], similar to importModuleMap*: Table[int, int] # (module.id, module.id) from #18366, which works.

arnetheduck · 2021-06-29T10:05:04Z

Not sure if they're bad or not, but re-exports are needed to get predictable behavior with generic code - the guideline we use is that if a public symbol uses a type declared in another module, that module should be reexported - it's the only way we've managed to alleviate unpredictable and surprising behavior (https://status-im.github.io/nim-style-guide/language.import.html) - also because in order to actually use the public symbols, you need those definitions anyway - ie Option is useless without get - specially with larger code-bases, it's somewhere between extremely tedious and impossible to get by without exports

timotheecour · 2021-07-16T00:27:20Z

IMO a pre-requisite for a durations module is nim-lang/RFCs#399, so that we're not stuck with a bad design forever.

stale · 2022-07-30T18:43:28Z

This pull request has been automatically marked as stale because it has not had recent activity. If you think it is still a valid PR, please rebase it on the latest devel; otherwise it will be closed. Thank you for your contributions.

konsumlamm · 2022-07-30T19:05:33Z

I don't think this is currently viable.

konsumlamm added 4 commits June 28, 2021 16:14

Split std/times in submodules

9f21c4b

Add module `std/times/core` Add module `std/times/durations`

Use {.all.} and importutils.privateAccess

5047012

Remove `{.raises: [Defect].}`

Fix typo, example, links

694fc33

Format doc comments

76fbdcd

Fix tests

e64928e

konsumlamm commented Jun 28, 2021

View reviewed changes

timotheecour reviewed Jun 28, 2021

View reviewed changes

timotheecour mentioned this pull request Jul 16, 2021

std/int128s and type Duration = distinct int128 nim-lang/RFCs#399

Open

ringabout mentioned this pull request Jan 14, 2022

Added std/oserrors for OS error reporting #19390

Merged

stale bot added the stale Staled PR/issues; remove the label after fixing them label Jul 30, 2022

konsumlamm closed this Jul 30, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split `std/times` #18378

Split `std/times` #18378

konsumlamm commented Jun 28, 2021 •

edited

Loading

timotheecour commented Jun 28, 2021 •

edited

Loading

konsumlamm commented Jun 28, 2021

konsumlamm Jun 28, 2021

timotheecour Jun 28, 2021

timotheecour Jun 28, 2021 •

edited

Loading

konsumlamm Jun 28, 2021

timotheecour Jun 28, 2021 •

edited

Loading

konsumlamm Jun 28, 2021

timotheecour Jun 28, 2021 •

edited

Loading

timotheecour Jun 28, 2021

konsumlamm Jun 28, 2021

timotheecour Jun 28, 2021

arnetheduck commented Jun 28, 2021

timotheecour commented Jun 28, 2021

Araq commented Jun 28, 2021

konsumlamm commented Jun 28, 2021 •

edited

Loading

timotheecour commented Jun 28, 2021 •

edited

Loading

Araq commented Jun 29, 2021

Araq commented Jun 29, 2021

timotheecour commented Jun 29, 2021

Araq commented Jun 29, 2021 •

edited

Loading

timotheecour commented Jun 29, 2021 •

edited

Loading

arnetheduck commented Jun 29, 2021

timotheecour commented Jul 16, 2021

stale bot commented Jul 30, 2022

konsumlamm commented Jul 30, 2022

Split std/times #18378

Split std/times #18378

Conversation

konsumlamm commented Jun 28, 2021 • edited Loading

timotheecour commented Jun 28, 2021 • edited Loading

konsumlamm commented Jun 28, 2021

konsumlamm Jun 28, 2021

Choose a reason for hiding this comment

timotheecour Jun 28, 2021

Choose a reason for hiding this comment

timotheecour Jun 28, 2021 • edited Loading

Choose a reason for hiding this comment

konsumlamm Jun 28, 2021

Choose a reason for hiding this comment

timotheecour Jun 28, 2021 • edited Loading

Choose a reason for hiding this comment

konsumlamm Jun 28, 2021

Choose a reason for hiding this comment

timotheecour Jun 28, 2021 • edited Loading

Choose a reason for hiding this comment

timotheecour Jun 28, 2021

Choose a reason for hiding this comment

konsumlamm Jun 28, 2021

Choose a reason for hiding this comment

timotheecour Jun 28, 2021

Choose a reason for hiding this comment

arnetheduck commented Jun 28, 2021

timotheecour commented Jun 28, 2021

Araq commented Jun 28, 2021

konsumlamm commented Jun 28, 2021 • edited Loading

timotheecour commented Jun 28, 2021 • edited Loading

Araq commented Jun 29, 2021

Araq commented Jun 29, 2021

timotheecour commented Jun 29, 2021

Araq commented Jun 29, 2021 • edited Loading

timotheecour commented Jun 29, 2021 • edited Loading

arnetheduck commented Jun 29, 2021

timotheecour commented Jul 16, 2021

stale bot commented Jul 30, 2022

konsumlamm commented Jul 30, 2022

Split `std/times` #18378

Split `std/times` #18378

konsumlamm commented Jun 28, 2021 •

edited

Loading

timotheecour commented Jun 28, 2021 •

edited

Loading

timotheecour Jun 28, 2021 •

edited

Loading

timotheecour Jun 28, 2021 •

edited

Loading

timotheecour Jun 28, 2021 •

edited

Loading

konsumlamm commented Jun 28, 2021 •

edited

Loading

timotheecour commented Jun 28, 2021 •

edited

Loading

Araq commented Jun 29, 2021 •

edited

Loading

timotheecour commented Jun 29, 2021 •

edited

Loading