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

Disable REPL hack in Julia 1.10 #866

Merged
merged 1 commit into from
May 10, 2023
Merged

Conversation

fingolfin
Copy link
Member

Resolves #864

@ThomasBreuer turns out this is much easier than we thought ;-)

@fingolfin fingolfin requested a review from ThomasBreuer May 8, 2023 11:10
@fingolfin fingolfin mentioned this pull request May 8, 2023
@codecov
Copy link

codecov bot commented May 8, 2023

Codecov Report

Merging #866 (5e22771) into master (9de7045) will increase coverage by 0.29%.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #866      +/-   ##
==========================================
+ Coverage   75.34%   75.63%   +0.29%     
==========================================
  Files          50       51       +1     
  Lines        4104     4117      +13     
==========================================
+ Hits         3092     3114      +22     
+ Misses       1012     1003       -9     
Impacted Files Coverage Δ
src/globals.jl 100.00% <100.00%> (ø)

... and 5 files with indirect coverage changes

@ThomasBreuer
Copy link
Member

I cannot reproduce this.

I have tried the nightly builds julia-1dcac5eb40 (10 days old) and julia-4e782bf91a (of today) with the proposed change.
In both versions, entering GAP.Globals.MTX.Is followed by <TAB> does not yield any output.
With Julia 1.8.5, this input yields the expected 10 matches which I get also in the GAP session.

Why do I get a different behaviour? (Is it because of the operating system (ubuntu 20.04) or because of the terminal (xfce), or what?)

@fingolfin
Copy link
Member Author

Argh. No, you are right, my mistake: I only tested GAP.Globals.IsP<TAB> as I forgot about the "nested" tab being the actual problem. Well, I also did run the test suite to verify it, knowing we had tests in it for the REPL completion... and the test suite passed!!!

... because we are not actually running the REPL completion tests 😠 so I just made PR #867 to fix that.

I'll resume looking for a proper solution. But in the meantime I will revamp this PR into simply disabling the REPL completion & tests for 1.10, to avoid the warnings when precompiling GAP.jl; we won't be worse off than we are this way.

@ThomasBreuer
Copy link
Member

Thanks.

But in the meantime I will revamp this PR into simply disabling the REPL completion & tests for 1.10, to avoid the warnings when precompiling GAP.jl; we won't be worse off than we are this way.

The pull request does this already, except for the comment

In Julia >= 1.10, the hack is not necessary anymore.

Verified

This commit was signed with the committer’s verified signature.
Aaron1011 Aaron Hill
@fingolfin
Copy link
Member Author

Updated

@fingolfin fingolfin changed the title Disable REPL hack in Julia 1.10, it is not needed anymore Disable REPL hack in Julia 1.10, May 8, 2023
@fingolfin fingolfin changed the title Disable REPL hack in Julia 1.10, Disable REPL hack in Julia 1.10 May 8, 2023
Copy link
Member

@ThomasBreuer ThomasBreuer left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks.

@fingolfin fingolfin merged commit 0dbd73c into oscar-system:master May 10, 2023
@fingolfin fingolfin deleted the mh/REPL branch May 10, 2023 06:58
@aviatesk
Copy link

aviatesk commented Jul 10, 2023

You can add the following patch to get reasonable completions for cases like GAP.Globals.MTX.I|:

diff --git a/src/globals.jl b/src/globals.jl
index 8ee3805..0b7a3ac 100644
--- a/src/globals.jl
+++ b/src/globals.jl
@@ -37,7 +37,7 @@ Main
 """
 const Globals = GlobalsType()
 
-function getproperty(::GlobalsType, name::Symbol)
+Base.@assume_effects :effect_free :terminates_globally function getproperty(::GlobalsType, name::Symbol)
     v = _ValueGlobalVariable(name)
     v === C_NULL && error("GAP variable $name not bound")
     return _GAP_TO_JULIA(v)

I am not sure if the getproperty method holds the declared effects though (please assert it yourself if you use this patch). And if that holds, the effects declaration should probably be in _ValueGLobalVariable and _GAP_TO_JULIA since it allows a broader optimization possibilities.

@fingolfin
Copy link
Member Author

OK, thanks for the hint! I am a bit wary of these annotations, after past experiments with @pure bit us. But these now new seem at least better documented, and after reading those comments, it seems we should indeed be able to use them here. Alas, they require Julia >= 1.8, but of course we can deal with that, too

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.

Warning on julia master
3 participants