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

add std/wintypes (for DWORD, etc) #344

Open
timotheecour opened this issue Feb 15, 2021 · 24 comments
Open

add std/wintypes (for DWORD, etc) #344

timotheecour opened this issue Feb 15, 2021 · 24 comments
Assignees

Comments

@timotheecour
Copy link
Member

timotheecour commented Feb 15, 2021

proposal

example

type DWORD* {.importc: "<IntSafe.h>".} = culong

benefits

  • avoids having to import winlean (a large dependency) just to import one such type
  • things like DWORD etc are defined in multiple places and often mis-typed, eg:
std/os: type DWORD = uint32
windows/winlean: type DWORD* = int32

note

the module should be std/wintypes, not windows/wintypes to abide to the rule that new modules are under std/; furthermore, the fact that it's windows-specific is already implicit in the module name (wintypes); the same argument was used for std/jsbigints.

links

@konsumlamm
Copy link

the module should be std/wintypes, not windows/wintypes to abide to the rule that new modules are under std/; furthermore, the fact that it's windows-specific is already implicit in the module name (wintypes); the same argument was used for std/jsbigints.

Is there anything speaking against something like std/windows/types (or std/win/types)? I think we should embrace "submodules" (subdirectories) more in the standard library, to avoid prefixing module names with what should be a subdirectory, to avoid namespace pollution and to make the standard library more structured.

I know it's too late now to change this now, but some examples from the standard library of what I have in mind:

  • std/asyncfile -> std/async/file
  • std/parsecsv -> std/parse/csv
  • std/jsbigints -> std/js/bigints

@metagn
Copy link
Contributor

metagn commented Feb 17, 2021

I know it's too late now to change this now, but some examples from the standard library of what I have in mind:

Not really. securehash was moved to std/sha1, could be done for other modules, don't even have to deprecate yet.

@Araq
Copy link
Member

Araq commented Feb 18, 2021

Is there anything speaking against something like std/windows/types

Yes, it sucks big time because you have to disambiguate the "types.nim" filename every time you access the file. Module names should be unique for clarity and navigability.

@metagn
Copy link
Contributor

metagn commented Feb 18, 2021

Yes, it sucks big time because you have to disambiguate the "types.nim" filename every time you access the file. Module names should be unique for clarity and navigability.

We can compensate for this by doing import std/os/wintypes instead (while still allowing import std/wintypes, like how collections works). async modules too aren't really related to each other beyond the fact that they use async. import std/[deques, math, parsecsv, algorithm] can look a little disorganized, I think grouping modules together could still be a good idea

@Araq
Copy link
Member

Araq commented Feb 18, 2021

I've probably written too much C# but I remember System.Text.RegularExpressions.Regex and it felt like the language was trolling me...

@timotheecour
Copy link
Member Author

timotheecour commented Feb 18, 2021

System.Text.RegularExpressions.Regex

no need to go to extremes, but I do find @hlaaftana 's suggestion interesting. Heres's how it could work:

  • rule 1: every new module still under std/
  • rule 2: keep file organization hierarchy to a minimum: std/windows/wintypes is ok but not much deeper
  • rule 3: keep file name unique even if redundant with dir prefix: (std/windows/wintypes, not std/windows/types)
  • use RFC: --path:prefix:path to resolve import prefix/suffix #291 (--path:prefix:path) for aliasing modules, and put those --path:prefix:path in config/nim.cfg, so that:
import std/windows/wintypes # ok
import std/wintypes # ok
import wintypes # error

import std/js/jsbigints # ok
import std/jsbigints # ok
  • the real file shall live in lib/std/windows/wintypes.nim, not lib/std/wintypes.nim (the latter doesn't need to exist, thanks to the --path:prefix:path feature)

example

as simple as adding this to $nim/config/nim.cfg:
path="std/wintypes:$lib/std/windows/wintypes.nim"
etc
(it currently contains entries like: path="$lib/windows")

@Araq
Copy link
Member

Araq commented Feb 18, 2021

I like your path feature but I wouldn't use it for even more aliasing. If import std/wintypes is ok, why also allow import std/windows/wintypes? Most people are happier without all these choices and objectively it makes the language harder to learn ("is std/windows/wintypes different from std/windows?")

@arnetheduck
Copy link

we're running into module (and symbol) name issues all the time in Nimbus due to short module names, global symbol pollution and the lack of namespacing in nim - hierarchies are easy to understand and organize anything that grows beyond trivial, so in addition to having a reasonable hierarchy in the std lib, it would be nice if the language itself was better prepared to handle hierarchical modules with the same leaf name - it's somewhat easy to control "global" uniqueness in the std lib because it's one monorepo project with one gatekeeper - beyond that, namespacing is the balanced approach between human-readable, no global arbiter and not using guids.. the big regex name in C# might look funny but the answer is not to throw out hierarchies - it's to ensure that hierarchies are easy to consume in code such that there's reasonable protection against name clashes.

@arnetheduck
Copy link

type DWORD* {.importc: "<IntSafe.h>".} = culong

importc isn't really needed?

@Araq
Copy link
Member

Araq commented Feb 24, 2021

Titus doesn't agree and works on sizable codebases, afaict, https://youtu.be/xu7q8dGvuwk?t=1624

@konsumlamm
Copy link

Titus doesn't agree and works on sizable codebases, afaict, https://youtu.be/xu7q8dGvuwk?t=1624

Idk much about C++, but this sounds like it's specific to deeply nested namespaces and C++...

@timotheecour
Copy link
Member Author

timotheecour commented Feb 24, 2021

ya his talk is specific to C++ namespaces, his arguments don't really apply to nim which has different lookup rules (and doesn't rely on include); module path namespace (std/windows/wintypes vs std/wintypes) is a separate topic from that talk.

Noone's arguing for std/platforms/windows/core/types here; but a reasonable amount of hierarchy is IMO good, so I'd be in favor of std/windows/wintypes instead of std/wintypes.

Note that I'm also in favor of encouraging unique module names, hence wintypes instead of types.

type DWORD* {.importc: "<IntSafe.h>".} = culong
importc isn't really needed?

maybe not in this case, but in other cases it'd probably be needed for portability.

@Araq
Copy link
Member

Araq commented Feb 25, 2021

Idk much about C++, but this sounds like it's specific to deeply nested namespaces and C++...

The arguments against both writability and readability were pretty universal and not C++ specific.

@arnetheduck
Copy link

The right solution rarely lies at the radical end of the spectrum.

That said, I'm describing a very practical and real problem with Nim development: independent developers will reuse the same name, both of modules and symbols, and there's no way to control that in a distributed ecosystem - it is however possible to improve the language such that it becomes less of a problem.

Adding a symbol in one module pollutes that name in all modules that use it, changing generics, mixins, overloads which in the benign case breaks compilation of that module, and in the not-so-benign case breaks runtime behavior. The same way, adding a module in one library breaks imports in other libraries (which is why we've started to strictly avoid unqualified imports).

Anyway, this wasn't meant to hijack this thread too much, except to point out that short unqualified names cause confusion and would ideally be avoided.

@Araq
Copy link
Member

Araq commented Mar 1, 2021

Adding a symbol in one module pollutes that name in all modules that use it, changing generics, mixins, overloads which in the benign case breaks compilation of that module, and in the not-so-benign case breaks runtime behavior. The same way, adding a module in one library breaks imports in other libraries (which is why we've started to strictly avoid unqualified imports).

Agreed but mostly unrelated, and suggestions for improvements are welcome. Really.

@arnetheduck
Copy link

FWIW, I still believe the way to go is to have system-specific versions of libraries in packages (nim-lang/Nim#5698) - this is a step in the right direction in that it replicates the C header files in Nim, more or less (in fact, the importc header is redundant and it's questionable if it's needed).

Above all, there's absolutely no reason why this should be in the standard library - the definitions in here have no relation to the Nim release cycle and are trivial in nature, meaning that they could be provided for Nim all the way back to 1.0, avoiding another false dependency.

@arnetheduck
Copy link

arnetheduck commented Mar 1, 2021

incidentally, the separate package also solves some of the namespace issues, because package names are mostly easier to keep unique and if the package uses it's own name in the expected imports, the problem takes care of itself - at that point, I'd structure a windows package by the same names that windows uses in MSDN - ie DWORD comes from basetsd.h (according to https://docs.microsoft.com/en-us/windows/win32/winprog/windows-data-types) - therefore, assuming the library is called win32, one would import win32/basetsd to get it - simple, not offensively nested or long, straightforward to remember, and reuses existing documentation "out there" to create a natural 1:1 mapping.

@arnetheduck
Copy link

oh, forgot - since this is a trivial package describing abi, we like to call them xxx_abi - they're great as starting points for wrappers on top of which one would build more natural nim code - so in this case ,the library would be called win32_abi

@timotheecour
Copy link
Member Author

timotheecour commented Mar 4, 2021

The same way, adding a module in one library breaks imports in other libraries (which is why we've started to strictly avoid unqualified imports).
Agreed but mostly unrelated, and suggestions for improvements are welcome. Really.

@Araq my proposed suggestion for this problem is syntax request: from std/[foo1,foo2] import nil ; from "." / [foo1,foo2] import bar1, bar2, bar3 · Issue #76 · nim-lang/RFCs

after which you'd be able to write:

from std/[sugar, sequtils, os] import toSeq, `=>`, `/`, 

because the following is verbose and people avoid it because of that:

from std/sugar import `=>`
from std/sequtils import toSeq
from std/os import `/`

and instead use unqualified impor tstd/[sugar, sequtils, os] which is a source of such problems when new symbols are introduced.

@arnetheduck I don't see how your suggestion of moving std/wintypes to a separate nimble package can help with stdlib modules that depend on std/wintypes; the point of std/wintypes is to gather in one place types that are currently scattered, duplicated (often with wrong types) in different stdlib modules. A more complete wrapper can however be defined as a stdlib module, and could import/re-export std/wintypes so that the types are fully compatible.

If we can't agree on where to put std/wintypes (mostly, std/wintypes vs std/windows/wintypes), then I'd suggest introducing std/private/wintypes for now so stdlib + nim repo can at least start using it, and then once the placement is settled, moving it to std/wintypes vs std/windows/wintypes in subsequent PR which would be an easy thing to do.

@narimiran
Copy link
Member

the following is verbose and people avoid it because of that

[citation needed]

@Araq
Copy link
Member

Araq commented Mar 5, 2021

@Araq my proposed suggestion for this problem is syntax request: from std/[foo1,foo2] import nil ; from "." / [foo1,foo2] import bar1, bar2, bar3 · Issue #76 · nim-lang/RFCs

That helps a little but I have in mind something like:

from tables import Table # allows for t.func but not for func(t)

var t: Table[string, int]

`[]=`(t, "key", 4) # invalid
t["key"] = 4 # valid

pairs(t) # invalid
t.pairs # valid

We should finally accept that attaching procs to types for abstract data types is essential and natural.

@timotheecour
Copy link
Member Author

the following is verbose and people avoid it because of that
[citation needed]

personal experience + very crude stats:
rg -t nim from lib | grep import | wc -l
101
rg -t nim import lib | wc -l
4923
nim --eval:'echo 101 / 4923'
0.0205159455616494

from tables import Table # allows for t.func but not for func(t)

this runs into https://nim-lang.org/docs/manual.html#templates-limitations-of-the-method-call-syntax, but in any case should be opt-in (for many reasons) eg:

from tables import Table {.attached.}

Note that it's compatible with #76 and both features can be used simultaneously if need be:

from std/[tables, sequtils, os, sugar] import `=>`, `/`, Table {.attached.}, toSeq

Note also that #76 is simpler / less can-of-worm-y.

@ringabout
Copy link
Member

Ref nim-lang/Nim#12327

@rockcavera
Copy link

This proposal certainly has to be accepted and put into practice. Those who develop on Windows and need to access the WINAPI, end up having to make the declaration of one type or another, and these declarations do not always end up being compatible with those of other users. The proposal itself points to divergences within Nim's own stdlib.

In addition to https://learn.microsoft.com/en-us/windows/win32/winprog/windows-data-types, there is this https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-dtyp/cca27429-5689-4a16-b2b4-9325d93e4ba2 which I believe better explained and perhaps more reliable(?).

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

8 participants