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

inference: minor refactor for compiler/reflection code #40745

Merged
merged 3 commits into from
May 13, 2021

Conversation

aviatesk
Copy link
Member

@aviatesk aviatesk commented May 7, 2021

No description provided.

@aviatesk aviatesk requested review from vtjnash and JeffBezanson May 7, 2021 14:56
base/reflection.jl Outdated Show resolved Hide resolved
aviatesk and others added 2 commits May 8, 2021 02:06
Co-authored-by: Jameson Nash <vtjnash@gmail.com>
@tkluck
Copy link
Contributor

tkluck commented May 28, 2021

Is the API break here intentional? E.g. FluxML/IRTools.jl#86 .

(Not sure what's the policy about renaming non-exported functions in Base. Feel free to disregard if this one is okay.)

@aviatesk
Copy link
Member Author

aviatesk commented May 29, 2021

Sorry breaking the code, but the function is neither exported nor documented, and so it's the responsibility of third-party
packages to keep the compatiblity with the Base internal.

I'd like to make sure to check an impact of renaming with JuliaHub's code search in the future, though.

@tkluck
Copy link
Contributor

tkluck commented May 29, 2021

My worry is: if we can find reverse dependencies in public code (like here) then there's also probably reverse dependencies in people's private code. It also seems like the rename is for cosmetic reasons, and that's not a very strong reason to risk it.

the function is neither exported nor documented, and so it's the responsibility of third-party packages to keep the compatiblity with the Base internal.

Is this rule documented? Elsewhere Base uses leading underscores to indicate which functions are for internal use. Is there any way to check if any of my code depends on non-API stable code?

Context: I had a similar situation pointed out to me in #34719. I just re-added the functions.

@aviatesk
Copy link
Member Author

the function is neither exported nor documented, and so it's the responsibility of third-party packages to keep the compatiblity with the Base internal.

Is this rule documented? Elsewhere Base uses leading underscores to indicate which functions are for internal use.

The documentation says "It is common to export names which form part of the API (application programming interface). ... However, since qualified names always make identifiers accessible, this is just an option for organizing APIs: unlike other languages, Julia has no facilities for truly hiding module internals."
So there is no absolute rule to judge what is "internal", but AFAIU non-exported (and non-documented) names in Base can be considered as Base's "internals" since it also has the explicit exports.

... But I'm not sure honestly. Base is different from any other module since it's used by almost all Julia programs, and so we may need to set a different discipline for it.
I'm totally fine to rename hasgenerator back to isgenerated (yes, it's really just a mere cosmetic change), but what I'm concerning is that this kind of restriction can be a frustration when developing Base, since it may mean we need to be conservative for future changes/enhancements to some extent.

@StefanKarpinski @JeffBezanson can either of you give us your opinion on this ? What's the canonical view on the line between Base's API and internals ?

Is there any way to check if any of my code depends on non-API stable code?

I think I can show you how we can extensibly detect "non-API stable" code as a plug-in analysis of a static analyzer of Julia in the very near future. I'll let you know.

@tkluck
Copy link
Contributor

tkluck commented May 30, 2021

@aviatesk appreciate your response and the potential for frustration if you can't make these kinds of changes. Let's see what Stefan/Jeff say.

JET.jl looks very interesting, I'll definitely keep an eye on it!

@KristofferC
Copy link
Member

We could just add back the original names that call the new ones together with a comment saying that they are for backwards compat. Similar things have been done many times before.

@aviatesk
Copy link
Member Author

aviatesk commented Jun 3, 2021

@tkluck So let me show off how we can detect those "unstable API"s automatically by a plug-in analysis of JET.

Here is the analysis result against IRTools@v0.4.3, and it correctly detects Base.isgenerated and others as expected:

# ... the definition of UnstableAPIAnalyzer here

julia> is_irtools(mod) = occursin("IRTools", string(Symbol(mod)))
julia> report_package("IRTools"; analyzer=UnstableAPIAnalyzer, is_target_module=is_irtools)
═════ 59 possible errors found ═════
┌ @ /Users/aviatesk/.julia/packages/IRTools/aSVI5/src/reflection/reflection.jl:39 Core.kwfunc(IRTools.Inner.invoke_meta)(Core.apply_type(Core.NamedTuple, (:world,))(Core.tuple(world)), IRTools.Inner.invoke_meta, T)
│┌ @ /Users/aviatesk/.julia/packages/IRTools/aSVI5/src/reflection/reflection.jl:69 IRTools.Inner.#invoke_meta#6(world, _3, T)
││┌ @ /Users/aviatesk/.julia/packages/IRTools/aSVI5/src/reflection/reflection.jl:74 Core.kwfunc(IRTools.Inner.meta)(Core.apply_type(Core.NamedTuple, (:types, :world))(Core.tuple(S, world)), IRTools.Inner.meta, T)
│││┌ @ /Users/aviatesk/.julia/packages/IRTools/aSVI5/src/reflection/reflection.jl:38 IRTools.Inner.#meta#1(types, world, _3, T)
││││┌ @ /Users/aviatesk/.julia/packages/IRTools/aSVI5/src/reflection/reflection.jl:43 Base._methods_by_ftype
│││││ Base._methods_by_ftype is unstable !: Base._methods_by_ftype
││││└─────────────────────────────────────────────────────────────────────────────────
││││┌ @ /Users/aviatesk/.julia/packages/IRTools/aSVI5/src/reflection/reflection.jl:49 Base.isgenerated
│││││ variable Base.isgenerated is not defined: Base.isgenerated
││││└─────────────────────────────────────────────────────────────────────────────────
││││┌ @ /Users/aviatesk/.julia/packages/IRTools/aSVI5/src/reflection/reflection.jl:49 Base.uncompressed_ast
│││││ Base.uncompressed_ast is unstable !: Base.uncompressed_ast
││││└─────────────────────────────────────────────────────────────────────────────────
││││┌ @ /Users/aviatesk/.julia/packages/IRTools/aSVI5/src/reflection/reflection.jl:54
... # many other "unstable API"s detected

You can have a look at JET.jl's documentation for more details or even try to run it yourself by checking out to the latest master of JET.jl.

Please note that the framework is very experimental at this point and not well documented (so it's not even tagged yet).

@tkluck
Copy link
Contributor

tkluck commented Jun 3, 2021

@aviatesk very cool! When it gets stable I'll be very happy to add it to the CI for all my packages.

shirodkara pushed a commit to shirodkara/julia that referenced this pull request Jun 9, 2021
johanmon pushed a commit to johanmon/julia that referenced this pull request Jul 5, 2021
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.

4 participants