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

Ensure Enumerable#to_a and Enumerable#tally properly retain return type of T #14447

Merged
merged 4 commits into from
Apr 12, 2024

Conversation

Blacksmoke16
Copy link
Member

Fixes #14440

@Blacksmoke16 Blacksmoke16 added kind:bug topic:stdlib:collection kind:regression Something that used to correctly work but no longer works labels Apr 6, 2024
@Blacksmoke16
Copy link
Member Author

Blacksmoke16 commented Apr 6, 2024

Welp, seems wasm and macos don't like this new spec?

Also seemingly run into #13193 when manually running std_spec:

$ make std_spec
Using /usr/bin/llvm-config [version=17.0.6]
./bin/crystal build -D strict_multi_assign -D preview_overload_order -Dwithout_interpreter  --exclude-warnings spec/std --exclude-warnings spec/compiler --exclude-warnings spec/primitives -o .build/std_spec spec/std_spec.cr
Using compiled compiler at .build/crystal
.build/std_spec --order=random
.build/std_spec: symbol lookup error: .build/std_spec: undefined symbol: __libc_start_main, version spec/std/spec/filters_spec.cr:124
make: *** [Makefile:106: std_spec] Error 127

But both indexable and enumerable specs pass for me.

@Blacksmoke16 Blacksmoke16 changed the title Ensure blockless #to_a returns T Ensure #itself properly retains T Apr 6, 2024
@straight-shoota straight-shoota added this to the 1.12.0 milestone Apr 6, 2024
@Blacksmoke16 Blacksmoke16 changed the title Ensure #itself properly retains T Ensure Enumerable methods properly retains return type of T Apr 6, 2024
@Blacksmoke16 Blacksmoke16 changed the title Ensure Enumerable methods properly retains return type of T Ensure Enumerable#to_a and Enumerable#tally properly retain return type of T Apr 6, 2024
@straight-shoota straight-shoota removed this from the 1.12.0 milestone Apr 8, 2024
@straight-shoota
Copy link
Member

straight-shoota commented Apr 8, 2024

Yeah, something's really funky about the failures on macos and wasm. How can this introduce an illegal instruction? 🤔

@Blacksmoke16
Copy link
Member Author

Blacksmoke16 commented Apr 9, 2024

I'm actually able to reproduce this on my intel mac with LLVM 17. Reproduces when running the whole of std_spec, or both specs together, but not when running the new modified specs on their own.

I'll play around with it and see if I can get a more minimal example...

Cut down the specs to:

enumerable_spec:

require "spec"

module SomeInterface; end

private record One do
  include SomeInterface
end

private record Two do
  include SomeInterface
end

private struct InterfaceEnumerable
  include Enumerable(SomeInterface)

  def each(&)
    yield One.new
    yield Two.new
  end
end

describe "Enumerable" do
  it "tallies an interface type" do
    InterfaceEnumerable.new.tally.should eq({One.new => 1, Two.new => 1})
  end
end

indexable_spec:

require "spec"

module OtherInterface; end

private record One do
  include OtherInterface
end

private record Two do
  include OtherInterface
end

private struct InterfaceIndexable
  include Indexable(OtherInterface)

  def size
    2
  end

  def unsafe_fetch(index : Int) : OtherInterface
    case index
    when 0 then One.new
    when 1 then Two.new
    else
      raise ""
    end
  end
end

describe Indexable do
  describe "#to_a" do
    it "without a block of an interface type" do
      InterfaceIndexable.new.to_a.should eq [One.new, Two.new]
    end
  end
end

Running them together:

$ ccrystal spec spec/std/indexable_spec.cr spec/std/enumerable_spec.cr --tap
Using compiled compiler at .build/crystal
ok 1 - Indexable(T) #to_a without a block of an interface type
Program exited because of an invalid instruction

However creating a single file with the contents of both does not.

@Blacksmoke16
Copy link
Member Author

Blacksmoke16 commented Apr 9, 2024

I think the gist of this is it's caused by having two private types typed as an interface interfering with each other when required from separate files.

E.g.

# test2.cr
module OtherInterface; end

private record One do
  include OtherInterface
end

private record Two do
  include OtherInterface
end

([One.new, Two.new] of OtherInterface) == [One.new, Two.new]

Then:

# test.cr
require "./test2"

module OtherInterface; end

private record One do
  include OtherInterface
end

private record Two do
  include OtherInterface
end

pp [One.new, Two.new] == [One.new, Two.new]

Results in:

$ crystal test.cr
Program exited because of an invalid instruction

If I had to guess, I'd say something regarding the "private to the file" thing isn't working out exactly as we expect giving one or the other files a reference to the other?

In regards to this PR, I'll rename the test types to not conflict and see if that helps.

@straight-shoota straight-shoota added this to the 1.13.0 milestone Apr 10, 2024
@straight-shoota straight-shoota merged commit 6128e72 into crystal-lang:master Apr 12, 2024
58 checks passed
@Blacksmoke16 Blacksmoke16 deleted the fix-to_a branch April 12, 2024 17:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:bug kind:regression Something that used to correctly work but no longer works topic:stdlib:collection
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enumerable(T)#to_a regression with interface type
2 participants