Don't use the Module(...) constructor to create a module

[asinghvi17]

On the advice of the compiler team, we should step away from using the Module(name) constructor and instead eval in a module via an Expr.

This simply changes the way that happens. Fortunately we have the module GC code already so that should flag us if something is going wrong.

This currently evals the module to Main, but there's no reason it couldn't eval to somewhere else.

[mortenpi]

This currently evals the module to Main, but there's no reason it couldn't eval to somewhere else.

It looks like Module(m) puts it into Main too, right? There's quite a bit of potentially fragile code that handles the module name, so I think it's best we keep in it Main.

[fingolfin]

This sounds like a good idea to me, but the test failures seem concerning :-(

[asinghvi17]

Yeah, not sure why the tests failed, but they magically turned green on the last commit. Might have been an intermittent thing, we'll see now I suppose

[fingolfin]

It turned green because you left no code changes in :-). I.e. this PR now only modifies the CHANGELOG (resulting in a merge conflict...)

[asinghvi17]

Ah no! I thought that would revert the commit, not the file 😅

[asinghvi17]

Hmm, it looks like wrapping here somehow changed printing for errors and vectors? Maybe it's the module declaration bringing in Base?

A lot of the errors seem fine to me, some are concerning though.

[fingolfin]

I took the liberty of squashing this into one commit, resolving the CHANGELOG problem, and applying runic. Let's see what the tests say now.

[fingolfin]

More weirdness in the log are these warnings:

WARNING: replacing module __atexample__named__1.
WARNING: replacing module __atexample__named__inlinesvg.
WARNING: replacing module __atexample__named__inlinehtml.
WARNING: replacing module __atexample__named__inlinepng.
WARNING: replacing module __atexample__named__inlinewebpgifjpeg.
WARNING: replacing module __atexample__named__showablelatex.
WARNING: replacing module __atexample__share_default_module__.
WARNING: replacing module __atexample__named__somename.
[ Info: CrossReferences: building cross-references.
┌ Warning: Cannot resolve @ref for md"[`unsafe_convert`](@ref Base.unsafe_convert)" in examples/src/lib/functions.md.
│ - No docstring found in doc for binding `Base.unsafe_convert`.
│ - No docstring found in doc for binding `Base.unsafe_convert`.
└ @ Documenter ~/work/Documenter.jl/Documenter.jl/src/utilities/utilities.jl:49

[fingolfin]

With PR #2804 things are much less confusing, and one can see what the actual issue(s) with this PR is/re. It seems this is one:

┌ Warning: failed to run `@example` block in examples/src/index.md:394-399
│ ```@example
│ write("issue793.jl", "\"Hello!\"")
│ r = include("issue793.jl")
│ rm("issue793.jl")
│ r
│ ```
│   exception =
│    SystemError: opening file "/Users/runner/work/Documenter.jl/Documenter.jl/test/examples/issue793.jl": No such file or directory
│    Stacktrace:
│      [1] include(mapexpr::Function, mod::Module, _path::String)
│        @ Base ./Base.jl:307

So before that, the custom include() method we inject into the module did its work; but now the one Julia provides does not. I do not yet know why, but at least this kind of issues is plausible, so that's progress

[asinghvi17]

Ah interesting! But yeah I would not have expected that to be the case, especially since we are eval'ing in to the module with invokelatest (at least I think we are)

Thanks for the effort getting this PR back into shape, by the way!

[asinghvi17]

So it turns out we were in fact not using invokelatest, let's see if that helps any in CI. I noticed you added the changes from #2804 here as well, which should make CI output much nicer to look at.

But maybe invokelatest won't help at all if this is some more bizarre issue. The path at least looks correct to me.

[fingolfin]

I don't think invokelatest has anything to do with it. But this might... look closer at the paths in the error:

┌ Warning: failed to run `@example` block in examples/src/index.md:394-399
│ ```@example
│ write("issue793.jl", "\"Hello!\"")
│ r = include("issue793.jl")
│ rm("issue793.jl")
│ r
│ ```
│   exception =
│    SystemError: opening file "/home/runner/work/Documenter.jl/Documenter.jl/test/examples/issue793.jl": No such file or directory

Note how it is looking for the file in test/examples/issue793.jl -- but the @example block is in test/examples/src/index.md so we looking at two different directories. Now of course the include function in a module always works relative to pkgdir(module), while the include we used to define just uses absdir, so it works relative to the current working directory. And actually if you think about it, that's also what include in the REPL does, and so arguably is what we need.

So we need to declare a custom include method after all. I am pushing that now.

[asinghvi17]

Awesome! Looks like all CI is passing with that change. Since the invokelatest was reverted I'd say this is ready to go!

[fingolfin]

Looks good to me now :-)

[mortenpi]

LGTM, thank you!