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

CmdRedirect constructor throwing TypeError #47180

Closed
giordano opened this issue Oct 15, 2022 · 2 comments · Fixed by #47181
Closed

CmdRedirect constructor throwing TypeError #47180

giordano opened this issue Oct 15, 2022 · 2 comments · Fixed by #47181
Labels
kind:regression Regression in behavior compared to a previous version
Milestone

Comments

@giordano
Copy link
Contributor

giordano commented Oct 15, 2022

BinaryBuilderBase.jl tests are failing on 532125d. A smaller reproducer cooked up with the help of https://github.com/maleadt/creduce_julia is:

function a(b; stdout )
    c = setenv(b, b.env)
    if true
        c = pipeline(c, stdout)
    end
    c
end
a(``; stdout)

The output is

ERROR: LoadError: TypeError: in new, expected Base.AbstractCmd, got a value of type Nothing
Stacktrace:
 [1] CmdRedirect
   @ ./cmd.jl:186 [inlined]
 [2] CmdRedirect
   @ ./cmd.jl:191 [inlined]
 [3] redir_out
   @ ./cmd.jl:355 [inlined]
 [4] #pipeline#761
   @ ./cmd.jl:389 [inlined]
 [5] pipeline
   @ ./cmd.jl:397 [inlined]
 [6] #a#3
   @ /tmp/foo.jl:4 [inlined]
 [7] top-level scope
   @ /tmp/foo.jl:8
in expression starting at /tmp/foo.jl:8

I don't see how you can enter

redir_out(src::AbstractCmd, dest::Redirectable) = CmdRedirect(src, dest, STDOUT_NO)
where src::AbstractCmd and then the CmdRedirect constructor complains src isn't AbstractCmd.

Last time tests passed was on 7f507e6.

@giordano
Copy link
Contributor Author

giordano commented Oct 15, 2022

git bisect blames 0620361 (from #47080). CC: @Keno

To reproduce the bisect run:

$ cat ./script.sh
#!/bin/bash

make cleanall || true
make -j40 USECCACHE=1 JULIA_PRECOMPILE=0 || exit 125

./usr/bin/julia -t1 --startup-file=no -e '
function a(b; stdout )
    c = setenv(b, b.env)
    if true
        c = pipeline(c, stdout)
    end
    c
end
a(``; stdout)
'
$ git bisect start
$ git bisect bad 532125d51d23f22c3fd117fe8a37c158fe16ac62
$ git bisect good 7f507e64aa14dbbd7e0c9b51a0ee72e7ea97bb19
$ git bisect run ./script.sh

@giordano giordano added this to the 1.9 milestone Oct 15, 2022
@Keno
Copy link
Member

Keno commented Oct 15, 2022

That commit itself is probably not bad, but if there's another unrelated bug that commit would have stopped hiding it. I'll turn on the oracle check and take a look.

Keno added a commit that referenced this issue Oct 16, 2022
The whole point of this pass is to compute and compare the
counts of all SSA value uses vs those of only-phi uses to
find SSA values that have no real uses. In #47080, I updated
the code to properly account for removal of phi edges in
the SSA count, but neglected to do the same in the phi-only
count, leading to #47180. Fix that.

Fixes #47180
Keno added a commit that referenced this issue Oct 16, 2022
The whole point of this pass is to compute and compare the
counts of all SSA value uses vs those of only-phi uses to
find SSA values that have no real uses. In #47080, I updated
the code to properly account for removal of phi edges in
the SSA count, but neglected to do the same in the phi-only
count, leading to #47180. Fix that.

Fixes #47180
Keno added a commit that referenced this issue Oct 17, 2022
The whole point of this pass is to compute and compare the
counts of all SSA value uses vs those of only-phi uses to
find SSA values that have no real uses. In #47080, I updated
the code to properly account for removal of phi edges in
the SSA count, but neglected to do the same in the phi-only
count, leading to #47180. Fix that.

Fixes #47180
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:regression Regression in behavior compared to a previous version
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants