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

Expand @static docstring #54206

Merged
merged 1 commit into from
Apr 24, 2024
Merged

Expand @static docstring #54206

merged 1 commit into from
Apr 24, 2024

Conversation

Sbozzolo
Copy link
Contributor

When I first learned about @static, I found its docstring not very clear or helpful. For example, I didn't understand that I had to use a conditional (yes, once you understand what @static does, it is obvious, but the reason I was reading the docstring is precisely because I didn't know how to use it). I also found the meaning of @static foo <&&,||> bar rather unclear.

I rephrased the docstring to something that I think is clearer and more helpful.

@giordano giordano added the docs This change adds or pertains to documentation label Apr 22, 2024
base/osutils.jl Outdated Show resolved Hide resolved
@LilithHafner
Copy link
Member

Thank you for catching starting a fix for this!

The docstring, as it was originally written, is not correct. The Julia execution order is

  1. Parsing
  2. Macro expansion
  3. Lowering
  4. Execution (an umbrella term for everything that follows)

The @static macro, being a macro, runs at the second phase. Consequently, it does not "Partially evaluate an expression at parse time."

What it actually does is evaluate the conditional (recursively macro expand, lower, and execute) and then, depending on the resulting value, returns only one (or zero) branch of the sub expression, deleting the rest of the branches before they are macro-expanded, lowered, or executed.

The key difference between that and normal conditionals is in the macro expansion and lowering phases.

Another quirk you should know about is that ccall(blah, blah, blah) is not a function call. This is strange and unexpected, and appears nowhere in the ccall docstring. But notice:

julia> ccall
ERROR: UndefVarError: `ccall` not defined in `Main`
Suggestion: check for spelling errors or missing imports.

julia> ccall(:jl_get_current_task, Ref{Task}, ()) # ccall is not defined, so this shouldn't work, right.....????
Task (runnable, started) @0x0000ffff19b20200 # Ha! ccall is special and confusing.

You can also see that it lowers differently than real functions

julia> Meta.lower(Main, :(foo(blah, blah, blah)))
:($(Expr(:thunk, CodeInfo(
    @ none within `top-level scope`
1%1 = foo
│   %2 = blah
│   %3 = blah
│   %4 = blah
│   %5 = (%1)(%2, %3, %4)
└──      return %5
))))

julia> Meta.lower(Main, :(ccall(blah, blah, blah)))
:($(Expr(:error, "ccall argument types must be a tuple; try \"(T,)\"")))

So, ccall magic happens, at least in part, during lowering which means it can be skipped by @static while it cannot be skipped by ordinary conditionals.

Aside, not relevant to this PR: Hopefully someday we can fix this quirk. In the meantime, a PR documenting the strange behavior of ccall would also be welcome, though ideally phrased in such a way as to allow ccall to become more like an ordinary function in the future


Does that help clarify what this macro does?

@Sbozzolo
Copy link
Contributor Author

Does that help clarify what this macro does?

Thank you very much for providing this comprehensive explanation! It definetely helps :)

I updated the docstring adding more information from @LilithHafner's message.

Copy link
Member

@LilithHafner LilithHafner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! This is a significant improvement over the existing docstring.

@LilithHafner LilithHafner added the merge me PR is reviewed. Merge when all tests are passing label Apr 24, 2024
such as a `ccall` to a non-existent function.
`@static if Sys.isapple() foo end` and `@static foo <&&,||> bar` are also valid syntax.
This is useful in cases where a construct would be invalid in some cases, such as a `ccall`
to a os-dependent function, or macros defined in packages that are not imported.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
to a os-dependent function, or macros defined in packages that are not imported.
to an OS-dependent function, or macros defined in packages that are not imported.


# Example

Suppose we want to parse an expression `expr` that is valid only on MacOS. We could solve
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Suppose we want to parse an expression `expr` that is valid only on MacOS. We could solve
Suppose we want to parse an expression `expr` that is valid only on macOS. We could solve


Suppose we want to parse an expression `expr` that is valid only on MacOS. We could solve
this problem using `@static` with `@static if Sys.isapple() expr end`. In case we had
`expr_apple` for MacOS and `expr_others` for the other operating systems, the solution with
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
`expr_apple` for MacOS and `expr_others` for the other operating systems, the solution with
`expr_apple` for macOS and `expr_others` for the other operating systems, the solution with

@LilithHafner LilithHafner merged commit 7860212 into JuliaLang:master Apr 24, 2024
6 of 8 checks passed
@LilithHafner
Copy link
Member

Oops! I only saw your comments when the page refreshed after I clicked merge. Sorry!

@LilithHafner LilithHafner removed the merge me PR is reviewed. Merge when all tests are passing label Apr 24, 2024
aviatesk pushed a commit that referenced this pull request Apr 25, 2024
Followup for premature merge of #54206

Implements @giordano's suggestions

cc @Sbozzolo
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs This change adds or pertains to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants