[~breaking] Fix issue where explicitly imported macros aren't detected as imported

[ericphanson]

closes #97

The added tests fail without the fix:

julia> include("test/issue_97_test.jl")
Issue #97: macro explicit imports: Error During Test at /Users/eph/ExplicitImports-2/test/issue_97_test.jl:10
  Test threw exception
  Expression: check_no_implicit_imports(Issue97, issue_path) === nothing
  ImplicitImportsException
  Module `Main.Issue97` is relying on the following implicit imports:
  * `@argcheck` which is exported by `Main.ArgCheck`
  
  Stacktrace:
   [1] check_no_implicit_imports(mod::Module, file::String; skip::Tuple{Module, Module, Module}, ignore::Tuple{}, allow_unanalyzable::Tuple{})
     @ ExplicitImports ~/.julia/packages/ExplicitImports/fjM9z/src/checks.jl:233
   [2] check_no_implicit_imports(mod::Module, file::String)
     @ ExplicitImports ~/.julia/packages/ExplicitImports/fjM9z/src/checks.jl:223
   [3] top-level scope
     @ ~/ExplicitImports-2/test/issue_97_test.jl:10
   [4] macro expansion
     @ ~/.julia/juliaup/julia-1.12.4+0.aarch64.apple.darwin14/share/julia/stdlib/v1.12/Test/src/Test.jl:1776 [inlined]
   [5] macro expansion
     @ ~/ExplicitImports-2/test/issue_97_test.jl:10 [inlined]
   [6] macro expansion
     @ ~/.julia/juliaup/julia-1.12.4+0.aarch64.apple.darwin14/share/julia/stdlib/v1.12/Test/src/Test.jl:677 [inlined]
Issue #97: macro explicit imports: Test Failed at /Users/eph/ExplicitImports-2/test/issue_97_test.jl:18
  Expression: getfield.(argcheck_usages, :analysis_code) == [ExplicitImports.IgnoredImportRHS, ExplicitImports.External]
   Evaluated: ExplicitImports.AnalysisCode[ExplicitImports.External, ExplicitImports.External] == ExplicitImports.AnalysisCode[ExplicitImports.IgnoredImportRHS, ExplicitImports.External]

Stacktrace:
 [1] top-level scope
   @ ~/ExplicitImports-2/test/issue_97_test.jl:10
 [2] macro expansion
   @ ~/.julia/juliaup/julia-1.12.4+0.aarch64.apple.darwin14/share/julia/stdlib/v1.12/Test/src/Test.jl:1776 [inlined]
 [3] macro expansion
   @ ~/ExplicitImports-2/test/issue_97_test.jl:18 [inlined]
 [4] macro expansion
   @ ~/.julia/juliaup/julia-1.12.4+0.aarch64.apple.darwin14/share/julia/stdlib/v1.12/Test/src/Test.jl:680 [inlined]
Test Summary:                     | Pass  Fail  Error  Total  Time
Issue #97: macro explicit imports |    1     1      1      3  0.1s
RNG of the outermost testset: Random.Xoshiro(0xf709e38b9d3d53d4, 0x75bf66b28f4c0d84, 0x9b0bae133ee651b7, 0x71fd9b1681755d7b, 0x7d71bba10fd6a350)
ERROR: LoadError: Some tests did not pass: 1 passed, 1 failed, 1 errored, 0 broken.
in expression starting at /Users/eph/ExplicitImports-2/test/issue_97_test.jl:7

[Copilot]

Pull request overview

This pull request fixes issue #97 where explicitly imported macros (like @argcheck) were incorrectly being flagged as implicit imports. The fix enables the import analysis to recognize macro names (K"MacroName" and K"StringMacroName") in addition to regular identifiers (K"Identifier") when analyzing import statements.

Changes:

• Modified analyze_import_type function to handle macro names in import statements
• Added comprehensive test coverage for the macro explicit import scenario

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File	Description
src/get_names_used.jl	Extended analyze_import_type to recognize K"MacroName" and K"StringMacroName" kinds in import analysis
test/issue_97.jl	Added test module with macro definition and explicit macro import to reproduce the reported issue
test/issue_97_test.jl	Added tests verifying that explicitly imported macros are correctly recognized and not flagged as implicit imports
test/runtests.jl	Integrated the new test suite into the main test runner

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[ericphanson]

@giordano Oceananigans integration test is failing here; looks like this bugfix has the effect of finding stale macro imports correctly whereas before I guess they were ignored. It looks like @unroll really is stale: https://github.com/search?q=repo%3ACliMA%2FOceananigans.jl%20path%3A%2F%5Esrc%5C%2FOrthogonalSphericalShellGrids%5C%2F%2F%20unroll&type=code.

This is unfortunate that it wasn't caught before so this will probably cause some test failures even though it is a bugfix. We could change this as v2 of ExplicitImports but not sure it's worth it.

[giordano]

Can you just skip that single module in the integration test?

[giordano]

BTW, I noticed @unroll was also stale in another module (not tested here, because I made it ExplicitImports-compliant after v0.103.1): CliMA/Oceananigans.jl#5135

[giordano]

Can you just skip that single module in the integration test?

Alternatively, until a new version of Oceananigans with that PR merged is tagged, how about running integration tests with Breeze.jl? It's a smaller codebase, but should be fully ExplicitImports-compliant already. Then when a new version of Oceananigans comes out you can restore the Oceananigans test.

In any case, please wait for CliMA/Oceananigans.jl#5135 to be merged before merging this PR, otherwise we'll suddenly get test failures 😬

[giordano]

Ok, now you can use commit CliMA/Oceananigans.jl@8a67381 via [sources] in https://github.com/JuliaTesting/ExplicitImports.jl/blob/4b0b3bffb884dc2a8b5550f1e47bf1d58b097f44/integration/Oceananigans/Project.toml

[ericphanson]

I asked on the open source Julia slack whether or not package maintainers think this should be a breaking change (ExplicitImports v2). There was a fair bit of discussion with most maintainers saying no, it's a bugfix semantically and therefore should be treated as one in semver versioning, with these additional reasons:

• maintainers do not want to have to rely on dependabot or compathelper to get v2 with fixes, they'd rather them immediately and without relying on external tooling
• they can pin ExplicitImports to an older patch if they need to
• some packages don't have compat bounds for test-deps so it wouldn't matter anyway (xref Aqua should check compat bounds for test/Project.toml Aqua.jl#361)
• if the package author wanted to have no e.g. stale explicit imports, then this is surfacing a bug in their code and should fail their tests

Though the main reason seemed to be that it is a bugfix and to follow normal semver.

I have my reservations with this; I think v2 could be a better route for the following reasons:

• if we make v2, then tests will fail on the dependabot PR, not the first PR post-bugfix, so the maintainer has more control over when to fix it (and easy to send an LLM to fix that). First PR post-bugfix could be from a new contributer and extra failures are difficult for them.
• ExplicitImports feels more "code style" to me than "correctness" so it's not important enough to merit "randomly" failing CI

Since the consensus is for it to be a patch release though, I think it's worth trying that and seeing if there is backlash against the tests starting to fail. We can always change approach next time.