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

[superseded] fix #14142: no more clash with: import os + use of existsDir/dirExists/existsFile/fileExists/findExe in config.nims #14143

Closed

Conversation

timotheecour
Copy link
Member

@timotheecour timotheecour commented Apr 28, 2020

/cc @Araq

future work

  • implicitly exported symols are not good, in particular when they differ from standard nim c behavior
  • user should import os for these and it'd work via vmops
  • we should be able to do this without much breakage by introducing a flag --nimsimports:off with semantics:
    --nimsimports:off # doesn't import anything, user has to import relevant module (eg os); will be default in some future tag 1.3.X
    --nimsimports:on # default until 1.3.x, gives a new warning while it's on

@Araq
Copy link
Member

Araq commented Apr 28, 2020

I spend effort on this error message, why remove it?

@timotheecour
Copy link
Member Author

timotheecour commented Apr 28, 2020

because the problems in #14142 are real and outweigh the benefits of having a nicer error message.

In following example, pkg/foo.nim (eg 3rd party code) can't be imported in some nimscript file because it'd give redefinition errors, same as #14142. Valid nim code should, as much as possible, be usable in nimscript without having to be modified with when defined(nimscript) sections.

example

nim e main.nims

# in main.nims (eg user config.nims or any other nims file)
import pkg/foo

# in pkg/foo.nim
import os
echo existsFile("/tmp/z01.txt") # Error: redefinition

{.error.} is the wrong pragma

We should instead use the new {.disabled.} pragma introduced in nim-lang/RFCs#213; if/when it's introduced, we can consider using:
{.pragma: noNimScript, disabled: "this proc is not available on the NimScript target".}

note: nimErrorProcCanHaveBody, introduced in #9665 (if n.sons[bodyPos].kind != nkEmpty and sfError notin s.flags: ...) improved {.error.} a bit but doesn't address the main problem, which causes #14142

…dirExists/existsFile/fileExists/findExe in config.nims
@timotheecour timotheecour force-pushed the pr_fix_14142_nims_existsDir branch from 9db29cb to 75ac5f8 Compare April 29, 2020 02:18
@timotheecour timotheecour changed the title fix #14142: no more clash with: import os + use of existsDir/dirExists/existsFile/fileExists/findExe in config.nims [superseded] fix #14142: no more clash with: import os + use of existsDir/dirExists/existsFile/fileExists/findExe in config.nims Jun 15, 2020
@timotheecour
Copy link
Member Author

superseded by #14658 , will be closed when #14658 is merged

@Araq
Copy link
Member

Araq commented Jun 15, 2020

The error message must stay.

@timotheecour
Copy link
Member Author

I don't understand why producing an error here is so important; what is your alternative proposal to #14658 to fix #14179, #14142 and the regression introduced in 3d2459b that makes CI 1.4X times slower (or more generally makes nim slower for any command, which especially matters for short lived commands like nim dump, inim, tests etc)

@timotheecour
Copy link
Member Author

superseded by #14658

@timotheecour timotheecour deleted the pr_fix_14142_nims_existsDir branch June 16, 2020 07:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants