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

Remove :rasgn and :mrasgn types. #150

Merged
merged 4 commits into from Nov 12, 2020
Merged

Remove :rasgn and :mrasgn types. #150

merged 4 commits into from Nov 12, 2020

Conversation

marcandre
Copy link
Contributor

@marcandre marcandre self-assigned this Nov 11, 2020
@marcandre
Copy link
Contributor Author

@eregon Test runs hang on truffleruby (second time in a row). I stopped last run after 5 hours (I'm surprised github actions doesn't seem to have a cutoff?)

@marcandre marcandre merged commit 2788a1a into rubocop:master Nov 12, 2020
@marcandre marcandre deleted the rasgn branch November 12, 2020 22:36
@eregon
Copy link
Contributor

eregon commented Nov 13, 2020

It's possible to specify a timeout for GitHub Actions:
https://docs.github.com/en/free-pro-team@latest/actions/reference/workflow-syntax-for-github-actions#jobsjob_idtimeout-minutes

I'll try to reproduce. It seems to only affect truffleruby-head, right?

@eregon
Copy link
Contributor

eregon commented Nov 13, 2020

It gets stuck in
https://github.com/rubocop-hq/rubocop-ast/blob/2788a1a0e129013f7e401a52b03a916b8dd0d495/lib/rubocop/ast/node_pattern/node.rb#L200-L204
(i.e., (0..Float::INFINITY).minmax takes forever)
Unfortunate that GitHub Actions doesn't send SIGINT or so, it would have shown that clearly.

TruffleRuby recently updated to 2.7, but didn't port that fix yet.

@eregon
Copy link
Contributor

eregon commented Nov 13, 2020

This is quite involved because I had to finish writing a spec for Range#minmax, synchronize specs across implementations and then actually fix it in TruffleRuby.

@eregon
Copy link
Contributor

eregon commented Nov 13, 2020

BTW, is there any reason not to simply use Range#begin/end here? It's very fragile to depend on RUBY_VERSION checks.

graalvmbot pushed a commit to oracle/truffleruby that referenced this pull request Nov 13, 2020
@marcandre
Copy link
Contributor Author

Thanks for the investigation!

is there any reason not to simply use Range#begin/end here

I don't remember, and it looks like all the arity ranges are inclusive, so maybe not. Still, I'd rather write things properly; also I should probably be using refinements here. Does map(&:refined_method) work in truffleruby?

It's very fragile to depend on RUBY_VERSION checks.

Agreed, it's also not the best in general. I think I can use a different check like (0..4.2).minmax.last

@eregon
Copy link
Contributor

eregon commented Nov 13, 2020

Fixed in oracle/truffleruby@df78798, thanks for the ping.
So no need to change anything :)

I don't remember, and it looks like all the arity ranges are inclusive, so maybe not.

Exclusive ranges wouldn't work anyway, 2.7.2:

> (0...Float::INFINITY).minmax
TypeError: cannot exclude non Integer end value

Does map(&:refined_method) work in truffleruby?

It does. But implementation-wise, that's a proper nightmare (need to cache globally per execution context & per refinements), I'd recommend having an explicit block. (actually TruffleRuby has an inline cache for it now, but still a global cache in the case the Symbol wasn't always the same, that's the part that's unpretty about this construct).

@marcandre
Copy link
Contributor Author

Fixed in oracle/truffleruby@df78798, thanks for the ping.
So no need to change anything :)

I don't remember, and it looks like all the arity ranges are inclusive, so maybe not.

Exclusive ranges wouldn't work anyway, 2.7.2:

Right. For parser's TreeRewriter, I break the integers into exclusive integer ranges and a last inifnite range (that I make inclusive because it doesn't work otherwise, but I think that's unfortunate): 0...15, 15...42, 42..Float::INFINITY. Works like a charm.

Does map(&:refined_method) work in truffleruby?

It does. But implementation-wise, that's a proper nightmare (need to cache globally per execution context & per refinements), I'd recommend having an explicit block.

Ok. Since TruffleRuby is fixed now, I could use a conditional refinement here to support MRI <= 2.6. Should be nicer.

rbotafogo pushed a commit to rbotafogo/truffleruby that referenced this pull request Dec 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants