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 staticFileExists and staticDirExists #22278

Merged
merged 1 commit into from
Aug 18, 2023
Merged

Conversation

demotomohiro
Copy link
Contributor

This PR partly fix #19414 .

In previus PR #22203, I tried to make fileExists and dirExists in std/private/oscommon works at compile-time even when --os:any option is set.
This PR adds staticos module that contains staticFileExists and staticDirExists that works like fileExists or dirExists but works only at compile-time even when --os:any option is set.

But I cannot make calling staticFileExists at runtime result in compile error even if I add compileTime pragma.
When I use when nimvm like this, error pragma is always cause error even if staticFileExists is called at compile-time.

proc staticFileExists*(filename: string): bool =
  when nimvm:
    discard
  else:
    {.error: "Don't run at runtime".}

@awr1
Copy link
Contributor

awr1 commented Jul 24, 2023

But I cannot make calling staticFileExists at runtime result in compile error even if I add compileTime pragma. When I use when nimvm like this, error pragma is always cause error even if staticFileExists is called at compile-time.

As-is the compile error for runtime gorge/slurp is emitted manually in the codegen pass.

@Varriount
Copy link
Contributor

I know what @Araq said in that previous PR, but I would like to know why there should be static<name> variants of these procedures over overloads that just work in a static context. From my point of view, it's much more natural for users to expect

static:
   let exists = fileExists(...)

to actually work, over having to use a special-cased version.

@awr1
Copy link
Contributor

awr1 commented Jul 25, 2023

@Varriount There are three real things that come to mind:

  1. that static* is technically "dangerous" as it implies arbitrary unsandboxed code to be executed at compile time, which can deserve being singled out in some security contexts.
  2. the caching option only really makes sense during builds
  3. staticExec/gorge has the potential to "not make sense" in certain cross-compilation contexts, e.g. trying and failing to execute a batch command on a Linux machine that targets Win32, which may appear to be "correct" based on a wrapped when defined(windows). Might be wrong but the I think the system.hostOS constant provide the compilation host, (but this is underspecified), meaning that Nim's convenience of "just moving code from runtime to CTFE" might be trickier when using using I/O. Those procedures being used at all require special care, ergo they are not conflated with the standard I/O ones.

@Varriount
Copy link
Contributor

@awr1 Fair enough. And I suppose the alternate behavior can always be implemented in the future, if necessary.

@demotomohiro
Copy link
Contributor Author

@awr1 Thank you!
Add new magic mStaticFileExists and mStaticDirExists to TMagic, handle them like mSlurp or mStaticExec in vmgen.nim and make them error in ccgexprs.nim.
Should it implemented in this PR?

@awr1
Copy link
Contributor

awr1 commented Jul 25, 2023

Should it implemented in this PR?

That would probably be best: don't forget jsgen.nim, which emits the same error.

@demotomohiro
Copy link
Contributor Author

Should it implemented in this PR?

That would probably be best: don't forget jsgen.nim, which emits the same error.

Thank you! I didn't aware jsgen.nim.

@demotomohiro
Copy link
Contributor Author

staticFileExists and staticDirExists are now compiler magic and calling them at runtime is a compile error.
But compiles(staticFileExists("")) returns true.

@Araq
Copy link
Member

Araq commented Aug 17, 2023

Sorry but commit 79b5d9f was much better, we don't add magics if we can avoid it.

@Araq Araq merged commit eb83d20 into nim-lang:devel Aug 18, 2023
19 checks passed
@github-actions
Copy link
Contributor

Thanks for your hard work on this PR!
The lines below are statistics of the Nim compiler built from eb83d20

Hint: mm: orc; opt: speed; options: -d:release
169248 lines; 8.683s; 611.441MiB peakmem

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

Successfully merging this pull request may close these issues.

Importing os module for use in macros fails when compiling for platform that doesn't support os
4 participants