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

make const a toplevel expr or error #12010

Closed
vtjnash opened this issue Jul 4, 2015 · 22 comments · Fixed by #25586
Closed

make const a toplevel expr or error #12010

vtjnash opened this issue Jul 4, 2015 · 22 comments · Fixed by #25586
Assignees
Labels
compiler:lowering Syntax lowering (compiler front end, 2nd stage) modules
Milestone

Comments

@vtjnash
Copy link
Member

vtjnash commented Jul 4, 2015

const can cause serious, subtle bugs when combined with precompilation. it is valid to use from the :toplevel scope, but it can cause problems if used at runtime (and it's already ignored when specified at the local scope. this could be fixed by warning for and ignoring the const declaration outside of the toplevel scope. or this could be fixed by internally distinguishing between const (as it is currently defined) and a new assigned once attribute (that provides none of the inference speed benefits of const, but just doesn't allow reassignment).

the following is fine:

module Fine
const binding = value
end

the following can confuse precompilation (and isn't great for type inference either):

module Bad
__init__() = (global const binding = value)
end

the following currently is ignored by parsing, so is neither bad nor good:

module Either
f() = (const localb = localv)
end
@vtjnash vtjnash added the priority This should be addressed urgently label Jul 4, 2015
@yuyichao
Copy link
Contributor

yuyichao commented Jul 4, 2015

the following can confuse precompilation (and isn't great for type inference either):

module Bad
__init__() = (global const binding = value)
end

I think this is still really useful for some packages that load shared libraries.

@yuyichao
Copy link
Contributor

yuyichao commented Jul 4, 2015

IIRC, the plan in #11456 (or a related one) is that the compiler should look inside the function and see if there's a global const defined inside and reserve a slot for it. (Looking for the specific comment now)

Edit: Comment about this is here #11456 (comment)

@vtjnash
Copy link
Member Author

vtjnash commented Jul 4, 2015

#11456 is about making global variable bindings resolved early, this thread is about disallowing the addition of new global const bindings. #11456 (comment) is exactly the subtle but serious issue that can happen if global const is allowed (since ccall will precompile a call to a static pointer, rather than a runtime lookup)

@timholy
Copy link
Member

timholy commented Jul 4, 2015

Interesting. What if we define an __init_pre__() and __init_post__(), and require that any global const assignments happen in post? Would that solve the problem?

EDIT: init_post runs after loading the cache. init_pre runs before caching.

@yuyichao
Copy link
Contributor

yuyichao commented Jul 4, 2015

(since ccall will precompile a call to a static pointer, rather than a runtime lookup)

Is this an issue if the pointer is used in the same module and __init__ (or __init_post__) is not run when the module is pre-compiled?

@vtjnash
Copy link
Member Author

vtjnash commented Jul 4, 2015

@timholy that's just:

module MyModule
__init_pre__() = "here1"
__init_post__() = "there2"

__init__() = __init_post__()
__init_pre__()
end

@yuyichao less so, but only because we don't run __init__ before precompiling the module, so it becomes actually just becomes undefined behavior at runtime due to invalidating type inference.

@yuyichao
Copy link
Contributor

yuyichao commented Jul 4, 2015

so it becomes actually just becomes undefined behavior at runtime due to invalidating type inference.

Sorry I forgot to mention that this should also be combined with letting typeinf/precompile look into functions (especially __init__) to see that the const global will be assigned at runtime.

@timholy
Copy link
Member

timholy commented Jul 4, 2015

I think I get it now. Presumably "only" affects functions that get JITted before the cache is written?

@timholy
Copy link
Member

timholy commented Jul 4, 2015

From an inference perspective, I'm guessing that much of the benefit of what we now use global const ... for would be obtained if we had an of_const_type declaration.

@vtjnash
Copy link
Member Author

vtjnash commented Jul 4, 2015

Sorry I forgot to mention that this should also be combined with letting typeinf/precompile look into functions (especially init) to see that the const global will be assigned at runtime.

@carnaval is that something typeinference could handle?

I think I get it now. Presumably "only" affects functions that get JITted before the cache is written?

JIT isn't really the right term, since this is AOT compiling. But since this is AOT, it's generally beneficial to do as much work as possible to build a good / complete cache.

@vtjnash
Copy link
Member Author

vtjnash commented Jul 4, 2015

for would be obtained if we had an of_const_type declaration...

module OfConstType
const binding = Ref{T}()

binding[] = 1
Base.Test.@test binding[] === 1
end

@timholy
Copy link
Member

timholy commented Jul 4, 2015

Sorry, I don't understand your example.

@yuyichao
Copy link
Contributor

yuyichao commented Jul 4, 2015

module OfConstType
const binding = Ref{T}()

binding[] = 1
Base.Test.@test binding[] === 1
end

I guess I wouldn't mind only allowing real const in global scope if we can making var::Type (edit: as global variable) a syntax sugar for this.

@vtjnash
Copy link
Member Author

vtjnash commented Jul 4, 2015

that seems to be Jeff's plan #11456 (comment)

@yuyichao
Copy link
Contributor

yuyichao commented Jul 4, 2015

@timholy

Sorry, I don't understand your example.

binding is a global const so it will always bind to the same object (type) and therefore binding[] always has the same type (edit: or whatever subset of types constraint by the type parameter T).

@yuyichao
Copy link
Contributor

yuyichao commented Jul 4, 2015

that seems to be Jeff's plan #11456 (comment)

Would still be nice if accessing them doesn't require adding [] though :).

@timholy
Copy link
Member

timholy commented Jul 4, 2015

Got it. Yes, that's a workable strategy.

@toivoh
Copy link
Contributor

toivoh commented Jul 4, 2015 via email

@yuyichao
Copy link
Contributor

yuyichao commented Jul 4, 2015

Hmm, even for typed global, it still won't cover this

__init__() = global const lib_path = find_library(...)

ccall((:sym, lib_path), ...)

@ViralBShah ViralBShah modified the milestones: 0.4.x, 0.6.0 Sep 10, 2016
@StefanKarpinski
Copy link
Member

This doesn't seem like a high priority given the lack of comment since July 2015.

@JeffBezanson
Copy link
Member

Might also be helped by #18933 ?

@StefanKarpinski
Copy link
Member

Resolved: we should do this.

@JeffBezanson JeffBezanson added compiler:lowering Syntax lowering (compiler front end, 2nd stage) modules labels Oct 2, 2017
@JeffBezanson JeffBezanson added triage This should be discussed on a triage call and removed triage This should be discussed on a triage call labels Jan 15, 2018
@JeffBezanson JeffBezanson self-assigned this Jan 16, 2018
visr added a commit to JuliaGeo/GDAL.jl that referenced this issue Aug 1, 2018
Jameson was right in JuliaLang/julia#12010, this can lead to subtle bugs. Fixes #49. Came to this because in julia 0.7 this is no longer allowed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:lowering Syntax lowering (compiler front end, 2nd stage) modules
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants