Nim stdlib style issues with --styleCheck:error

Summary

Nim's stdlib currently errors in several places when compiling with --styleCheck:error. This can cause errors when compiling code that uses certain parts of the standard library.

Description

Add a config.nims file in the root of nim's repository that contains --styleCheck:error, and fix any style issues within the standard library.

Alternatives

Alternatively, we can get rid of styleCheck completely. As it stands, it's completely useless in many cases simply because the standard library will cause style check errors. I've had several projects fail to compile because I've imported a stdlib module, despite compiling fine without the config.nims file.

Additional Information

Currently both nim and nimble fail to compile due to stdlib issues. Nim's is several places (I haven't found all of them), while nimble's issues lie in lib/pure/net.nim.

I understand this may seem like a nitpick, but I think the compiler shouldn't provide a style checking option that causes the compiler to fail to compile itself.

[Araq]

I went through the stdlib with --styleCheck:error when I implemented it so I wonder what's failing. Can't be much?

I don't think it is. I'll go through it today when I have some time and report back.

[kaushalmodi]

@Araq

I went through the stdlib with --styleCheck:error when I implemented it so I wonder what's failing.

Should the --styleCheck:error be then added to the repo's .cfg or .nims so that the CI catches regressions with styleCheck incompatibility?

[kaushalmodi]

@3n-k1 It's always useful to show the recipe to reproduce an error in bug reports :)

[Araq]

@kaushalmodi It's active for the Nim compiler itself, not for the default config as we don't know if that should really be the new default.

@kaushalmodi sorry. I literally just ran echo '--styleCheck:error' >> config.nims && bash build_all.sh

I'm out of class now, so I can take a look at any issues.

looks like a lot of the issues lie in the net and posix modules.

The style checker also has some features I take issue with. For instance, in lib/system.nim, line 2262 (the first error when building with style checking), the style checker is complaining that csize_t should be csizeT, despite the name being declared as the former. I think snake case is a perfectly proper naming convention, especially because the name is taken from C's size_t type.

More work might need to go into the style checker to make it more robust. @Araq @kaushalmodi I'd definitely like your opinion on this, especially because "fixing" other issues with regards to styleCheck vs the stdlib would break a lot of existing code.

[Araq]

The style checker also has some features I take issue with. For instance, in lib/system.nim, line 2262 (the first error when building with style checking), the style checker is complaining that csize_t should be csizeT, despite the name being declared as the former.

That's an issue because you haven't bootstrapped properly. There is a special rule for csize_t in the style checker.

I think snake case is a perfectly proper naming convention, especially because the name is taken from C's size_t type.

Yeah but I don't think so. It's an ugly legacy and not Nim's style.

looks like a lot of the issues lie in the net and posix modules.

Here is what posix.nim contains:

when defined(nimHasStyleChecks):
  {.push styleChecks: off.}

Maybe all your problems are coming from an old Nim binary that lacks nimHasStylechecks?

I was using the 1.0.0 release from choosenim, but apparently that was a bit too far out of date. Bootstrapping and adding the bin/ directory to the beginning of my path worked.

I've finished most of the stdlib. It seems testament has some issues too though. When I send a PR, are you interested in everything, or just the stdlib fixes?

[Araq]

In everything, however, don't adapt the tests.

[D-Nice]

Here is one quick instance where I ran into this:

/nim/lib/pure/strutils.nim(961, 26) Error: 'sizeOf' should be: 'sizeof'
    toHex(BiggestInt(x), T.sizeOf * 2)

I'll have to revert it to warnings instead of error for now in my CI.

[D-Nice]

As an addendum, switching the flag from error to hint for styleCheck causes none of the subsequent stdlib issues. However, it also doesn't show any purposeful style errors I add in (which it does show with the error switch), and that's with the following additional args --hints:on --verbosity:2.

Hope I'm not hijacking but unsure if I'm missing something, or if I should indeed open an issue regarding this?

Sorry Araq, I missed your reply. I'll have a PR ready tonight.

@D-Nice I would open a separate issue. You aren't hijacking anything, but that definitely seems like something bad enough to warrant its own thread.