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

[release 1.10] effects wrong for function that throws MethodError #53259

Closed
bvdmitri opened this issue Feb 9, 2024 · 8 comments
Closed

[release 1.10] effects wrong for function that throws MethodError #53259

bvdmitri opened this issue Feb 9, 2024 · 8 comments
Assignees

Comments

@bvdmitri
Copy link
Contributor

bvdmitri commented Feb 9, 2024

Consider the following code

foreach(1:10) do 
    println(i)
end

This fails, because I forgot to define the i name after the do block.

MethodError: no method matching (::var"#11#12")(::Int64)

Closest candidates are:
  (::var"#11#12")()
   @ Main In[26]:2

The error above is expected. However the following example does not even produce any error.

struct TestStruct
    size::Int
    someotherfield
end

function TestStruct(size::Int)
    foreach(1:size) do 
        println(i)
    end
    return TestStruct(size, nothing)
end

TestStruct(10) # returns `TestStruct(10, nothing)` without any error

I would expect the second example to fail as well. This leads to some nasty undebuggable behaviour since nothing points to the problematic place. Basically Julia arbitrarily skips a potentially important piece of logic in the constructor for the TestStruct.

Julia Version 1.10.0
Commit 3120989f39b (2023-12-25 18:01 UTC)
Build Info:
  Official https://julialang.org/ release
Platform Info:
  OS: macOS (arm64-apple-darwin22.4.0)
  CPU: 10 × Apple M2 Pro
  WORD_SIZE: 64
  LIBM: libopenlibm
  LLVM: libLLVM-15.0.7 (ORCJIT, apple-m1)
  Threads: 10 on 6 virtual cores
Environment:
  JULIA_NUM_THREADS = 4,4
@bvdmitri
Copy link
Contributor Author

bvdmitri commented Feb 9, 2024

# @code_lowered TestStruct(10)
CodeInfo(
1#9 = %new(Main.:(var"#9#10"))%2 = #9%3 = 1:size
│        Main.foreach(%2, %3)
│   %5 = Main.TestStruct(size, Main.nothing)
└──      return %5
)
# @code_llvm TestStruct(10)
;  @ In[24]:1 within `TestStruct`
define void @julia_TestStruct_4659({ i64, {}* }* noalias nocapture noundef nonnull sret({ i64, {}* }) align 8 dereferenceable(16) %0, [1 x {}*]* noalias nocapture noundef nonnull align 8 dereferenceable(8) %1, i64 signext %2) #0 {
top:
;  @ In[24]:5 within `TestStruct`
  %3 = getelementptr inbounds [1 x {}*], [1 x {}*]* %1, i64 0, i64 0
  store {}* inttoptr (i64 4368531464 to {}*), {}** %3, align 8
  %.repack = getelementptr inbounds { i64, {}* }, { i64, {}* }* %0, i64 0, i32 0
  store i64 %2, i64* %.repack, align 8
  %.repack1 = getelementptr inbounds { i64, {}* }, { i64, {}* }* %0, i64 0, i32 1
  store {}* inttoptr (i64 4368531464 to {}*), {}** %.repack1, align 8
  ret void
}
# @code_native TestStruct(10)
	.section	__TEXT,__text,regular,pure_instructions
	.build_version macos, 14, 0
	.globl	_julia_TestStruct_4907          ; -- Begin function julia_TestStruct_4907
	.p2align	2
_julia_TestStruct_4907:                 ; @julia_TestStruct_4907
; ┌ @ In[24]:1 within `TestStruct`
; %bb.0:                                ; %top
	mov	x9, #32776
	movk	x9, #1122, lsl #16
	movk	x9, #1, lsl #32
; │ @ In[24]:5 within `TestStruct`
	str	x9, [x0]
	stp	x1, x9, [x8]
	ret
; └
                                        ; -- End function
.subsections_via_symbols

@bvdmitri
Copy link
Contributor Author

bvdmitri commented Feb 9, 2024

Reduced to the following

function foo(size)
    foreach(1:size) do 
        println(i)
    end
end

function foo()
    foreach(1:10) do 
        println(i)
    end
end
foo(10) # returns without errors
foo() # errors

@bvdmitri
Copy link
Contributor Author

bvdmitri commented Feb 9, 2024

I think this is wrong

# @code_llvm foo(10)
;  @ In[75]:1 within `foo`
define void @julia_foo_5731(i64 signext %0) #0 {
top:
  ret void
}

In addition

Base.infer_effects(foo, (Int,))
(+c,+e,+n,+t,+s,+m,+i)

Base.infer_effects(foo, ())
(+c,+e,!n,+t,+s,+m,+i)

@vtjnash
Copy link
Member

vtjnash commented Feb 9, 2024

This seems fixed on v1.11 / master, but I am not sure which of the effect-fix PRs would have fixed it and needs to be backported

@vtjnash vtjnash added the backport 1.10 Change should be backported to the 1.10 release label Feb 9, 2024
@vtjnash vtjnash changed the title Confusing behaviour for the do syntax with missing argument name [release 1.10] effects wrong for function that throws MethodError Feb 9, 2024
@bvdmitri
Copy link
Contributor Author

bvdmitri commented Feb 12, 2024

The fix is bisected to #50805 (first time I used automatic bisect run 'script', but the PR makes sense to me)

@Keno @vtjnash

With this script

bisectscript.jl
function foo(size)
           try
           foreach(1:size) do
               println(i)
           end
           catch e
               return 1
           end
           return 0
       end

if foo(10) === 0
	exit(0)
else
	exit(1)
end

@bvdmitri
Copy link
Contributor Author

Also the current issue is very similar to this one #53062

@aviatesk
Copy link
Member

aviatesk commented Feb 12, 2024

This will be fixed on v1.10.1. In particular, #53076 fixed this issue.

@aviatesk
Copy link
Member

Thanks for your report and bisect, @bvdmitri !

@aviatesk aviatesk removed the backport 1.10 Change should be backported to the 1.10 release label Feb 12, 2024
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

No branches or pull requests

4 participants