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

Main entrypoint take 3 - revenge of the macro #51435

Merged
merged 1 commit into from Oct 8, 2023
Merged

Main entrypoint take 3 - revenge of the macro #51435

merged 1 commit into from Oct 8, 2023

Conversation

Keno
Copy link
Member

@Keno Keno commented Sep 23, 2023

As they say, if at first you don't succeed, try again, then try again, add an extra layer of indirection and take a little bit of spice from every other idea and you've got yourself a wedding cake. Or something like that, I don't know - at times it felt like this cake was getting a bit burnt.

Where was I?

Ah yes.

This is the third edition of the main saga (#50974, #51417). In this version, the spelling that we'd expect for the main use case is:

function (@main)(ARGS)
    println("Hello World")
end

This syntax was originally proposed by @vtjnash. However, the semantics here are slightly different. @main simply expands to main, so the above is equivalent to:

function main(ARGS)
    println("Hello World")
end
@main

So @main is simply a marker that the main binding has special behavior. This way, all the niceceties of import/export, etc. can still be used as in the original Main.main proposal, but there is an explicit opt-in and feature detect macro to avoid executing this when people do not expect.

Additionally, there is a smooth upgrade path if we decide to automatically enable Main.main in Julia 2.0.

@adienes
Copy link
Contributor

adienes commented Sep 23, 2023

regardless of which approach gets chosen, I appreciate that you are so responsive to & take action on all the (probably frustrating) criticisms!

fwiw, from a user POV, I (think) I like this the most out of the three proposals

@digital-carver
Copy link
Contributor

The text in NEWS.md (and the manual docs to a lesser extent) focus heavily on the entrypoint being Main.main, mentioning the @main macro much less than Main.main. Since @main is the interface to this functionality for the user, it would be clearer to describe things in terms of the macro, mentioning the Main.main implementation detail only in places where it matters (eg. for explaining "The main binding may be imported from a package.")

@Keno
Copy link
Member Author

Keno commented Sep 23, 2023

Well, that's just because I took the text from the old old PR and just tweaked it to be accurate. If we go with this, we'll have the full rest of the release cycle to tweak the text :).

@Seelengrab
Copy link
Contributor

I like it! Should we run PkgEval, just to make sure nothing breaks by accident again? 😅

Copy link
Contributor

@MasonProtter MasonProtter left a comment

Choose a reason for hiding this comment

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

I like this idea, it seems like to me that it's a very clever compromise that satisfies pretty much all of the substantive concerns that were raised in previous rounds.

Below are a couple thoughts I had reading this, but they're not strongly held opinions, just thought I'd put it out there.

base/client.jl Outdated
Core.eval(__module__, quote
# Force the binding to resolve to this module
global main
global var"#__main_is_entrypoint__#" = true
Copy link
Contributor

Choose a reason for hiding this comment

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

should this maybe be const? Or at least type asserted? That way should_use_main_entrypoint() isn't accessing an untyped global.

Copy link
Member Author

Choose a reason for hiding this comment

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

Type assert is reasonable. const, I wanted to allow the possibility of turning it back off through some mechanism as a possible future extension, though of course it's internal, so we can always change it. I'll add the type assert though.

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
global var"#__main_is_entrypoint__#" = true
global var"#__main_is_entrypoint__#"::Bool = true

just bumping this suggestion before merge.

Comment on lines +662 to +663
if Base.binding_module(__module__, :main) !== __module__
error("USAGE: Symbol `main` is already a resolved import in module $(__module__). `@main` must be used in the defining module.")
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should allow @main to be also used in Main. i.e. something like

Suggested change
if Base.binding_module(__module__, :main) !== __module__
error("USAGE: Symbol `main` is already a resolved import in module $(__module__). `@main` must be used in the defining module.")
if Base.binding_module(__module__, :main) !== __module__ || __module__ === Main
error("USAGE: Symbol `main` is already a resolved import in module $(__module__). `@main` must be used in the defining module, or in Main.")

That way someone can alternatively do

module MyApp

export main
main(ARGS) = println("Hello World")

end
using .MyApp
@main 

if they want. This is arguably a bit more explicit and clear, since it kinda makes me nervous to using a module and have that silently define an entrypoint.

I guess though that the downside of this would be that it wouldn't map nicely onto a hypotheical 2.0 world where @main just becomes unadorned main.

Copy link
Member Author

Choose a reason for hiding this comment

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

I considered it, but I'd prefer not to for the moment. You can always do const (@main) = MyApp.main for the above case.

@Keno
Copy link
Member Author

Keno commented Sep 26, 2023

@nanosoldier runtests()

@Keno
Copy link
Member Author

Keno commented Sep 26, 2023

I did some additional socialization of this an dit sounds like people are generally happy with this, so I'm planning to merge this version once PkgEval is back.

@nanosoldier
Copy link
Collaborator

The package evaluation job you requested has completed - possible new issues were detected.
The full report is available.

@Keno
Copy link
Member Author

Keno commented Sep 27, 2023

Looks like the only package that has an issue is Comonicon.jl, because it also has an export called @main. Not technically an issue because our policy allows adding additional names, but let's see if we can deconflict. @Roger-luo, would you consider renaming the Comonicon macro to @automain or something?

@Roger-luo
Copy link
Contributor

would you consider renaming the Comonicon macro to @automain or something?

Well, this API is already in the 1.x version of Comonicon, so I cannot change it and IMO this means Julia 1.x cannot change the behavior either. Otherwise, this will break everything that depends on it.

But I assume this is fine because when you do

using Comonicon

it should overwrite the exports from Base? Or this would error? If these errors won't this break the Julia's promise of 1.x compatibility?

@Keno
Copy link
Member Author

Keno commented Sep 27, 2023

Or this would error?

You get an ambiguity error, and it'd need an explicit

using Comonicon: @main to override

If these errors won't this break the Julia's promise of 1.x compatibility?

No, new exports are allowed under our compatibility policy in minor releases. There's a few new ones every release.

@Roger-luo
Copy link
Contributor

No, new exports are allowed under our compatibility policy in minor releases. There's a few new ones every release

but won't this as a result cause people depend on the older Comonicon version to error even if I made the change? That means the code that runs in the previous Julia version stops running in the later Julia version. I can ask people to do an explicit import while using Comonicon in the future or I can figure out a different 4-letter name. But I'm more concerned about this de-facto-breaking change on who is already using the package.

@Keno
Copy link
Member Author

Keno commented Sep 27, 2023

Yes, unfortunately, we don't currently have a way to avoid that. Same with with and @with that a few packages were using from the ScopedValue PR. With the number of packages we have now, it's probably gonna start happening a lot more. We probably need to figure out some sort of versioned export system, but that's a bit of a separate issue from this PR.

@DilumAluthge
Copy link
Member

Isn't this just #42080?

People should not be doing using Foo (except maybe in ephemeral REPL sessions).

@Keno
Copy link
Member Author

Keno commented Sep 27, 2023

Isn't this just #42080?

yes

if !isempty(args)
error("USAGE: `@main` is expected to be used as `(@main)` without macro arguments.")
end
if isdefined(__module__, :main)
Copy link
Sponsor Member

@vtjnash vtjnash Sep 28, 2023

Choose a reason for hiding this comment

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

Ideally we could return this code as

Expr(:toplevel_but_last,
   quote everything end,
   esc(:main) )

instead of running it, so as to avoid having the macro itself do side-effects and throw error

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know if lowering is really set up to do this, as it'd have to be able to hoist that toplevel expression. @JeffBezanson ? But yes, if possible that would be better of course.

@kdheepak
Copy link
Contributor

kdheepak commented Sep 29, 2023

What happens in this case?

# Developer 1's package
module Package1
export main
(@main)(ARGS) = println("Hello World!")
end

# Developer 2's package
module Package2
export main
(@main)(ARGS) = println("Goodbye World!")
end

# user code
using Package1
using Package2

Will users will get a warning like this with no function being executed?

WARNING: both Package1 and Package2 export "main"; uses of it in module Main must be qualified

I suspect even if users defined a (@main) it'll still give the same warning? Is there a way around that?

@Keno
Copy link
Member Author

Keno commented Sep 29, 2023

What happens in this case?

Usual symbol visibility rules apply. In the specific case case mentioned, users get a warning, if there's an @main in Main, it's preferred as usual.

@kdheepak
Copy link
Contributor

I feel like something like this is going to be fairly common:

# popular dependency specified in Project.toml
module PopularPackage
export main
(@main)(ARGS) = println("Hello World!")
end

# user code in ./src/UserPackage.jl
module UserPackage
export main
(@main)(ARGS) = println("Goodbye World!")
end

# user code in ./bin/script.jl
using PopularPackage
using UserPackage

I was wondering if there was a way to automate running UserPackage.main() in this case.

@Keno
Copy link
Member Author

Keno commented Sep 29, 2023

I was wondering if there was a way to automate running UserPackage.main() in this case.

using UserPackage: main or import PopularPackage, but I don't think it's all that common, we'd likely discourage mixing exports of main and other symbols, so there wouldn't be a reason to using PopularPackage in the same script.

@Keno
Copy link
Member Author

Keno commented Oct 3, 2023

I plan to merge this in a day or so. Something to address the #42080 issue is being discussed separately in general, but TBD whether something will be doable there.

As they say, if at first you don't succeed, try again, then
try again, add an extra layer of indirection and take a little
bit of spice from every other idea and you've got yourself a wedding
cake. Or something like that, I don't know - at times it felt like
this cake was getting a bit burnt.

Where was I?

Ah yes.

This is the third edition of the main saga (#50974, #51417). In
this version, the spelling that we'd expect for the main use case is:
```
function (@main)(ARGS)
    println("Hello World")
end
```

This syntax was originally proposed by `@vtjnash`. However, the semantics
here are slightly different. `@main` simply expands to `main`, so the above
is equivalent to:
```
function main(ARGS)
    println("Hello World")
end
@main
```

So `@main` is simply a marker that the `main` binding has special behavior.
This way, all the niceceties of import/export, etc. can still be used
as in the original `Main.main` proposal, but there is an explicit
opt-in and feature detect macro to avoid executing this when people
do not expect.

Additionally, there is a smooth upgrade path if we decide to automatically
enable `Main.main` in Julia 2.0.
@Keno Keno merged commit f90fd72 into master Oct 8, 2023
5 of 6 checks passed
@Keno Keno deleted the kf/main3 branch October 8, 2023 23:10
@PallHaraldsson
Copy link
Contributor

PallHaraldsson commented Oct 10, 2023

@Keno I don't see the startup.jl file mentioned, i.e. that it's run or NOT run. I believe it was actually a mistake to run it (at least for non-interactive use). My understanding with this @main macro is that it will be helpful for PackageCompiler.jl apps, which I think absolutely shouldn't run the file (if they actually do), and this is also for scripts where I feel the same; or at least if you want same behaviour for both, which I also think was the point.

Since this was only merged yesterday to (not yet stable) master, I think opting out of the file can still be added. And documented. Then people who actually like running the file by default can just not use the macro, or it could add an option for that:

@main startup-file=yes ...

I'm not pushing for that non-default option, I don't think it's really needed. I'm not sure if it should be the default for when running from the REPL (EDIT: I suppose it will since then file will have already been run when you include a file using @main), however, or just also keep same consistent behaviour there.

JuliaPy/PythonCall.jl#376

@Keno
Copy link
Member Author

Keno commented Oct 10, 2023

The startup.jl thing will need to be an option to the compiler. It doesn't make sense on the @main macro which needs to have semantics for both interactive and compiled uses cases and for the interactive one, it comes too late to load startup.jl.

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.

None yet