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

+ ruby28.y: endless method definition #676

Merged
merged 3 commits into from Apr 29, 2020

Conversation

palkan
Copy link
Contributor

@palkan palkan commented Apr 16, 2020

TBD

@iliabylich
Copy link
Collaborator

I think it's too early to start implementing these new features at least because I'm not sure what will be the next MRI version. Are you sure it's going to be 2.8 and not 3.0?

@palkan
Copy link
Contributor Author

palkan commented Apr 16, 2020

Are you sure it's going to be 2.8 and not 3.0?

Currently, it's named 2.8 🤷🏻‍♂️

I'm going to implement these (controversal) features for Ruby Next as soon as possible 🙂.

The purpose of this PR is to discuss the API to make it possible to merge these features to Parser easily when 3.0 became stable. And to let you know that I plan to work on this)

Probably, it's better to open a PR in https://github.com/ruby-next/parser and link to the issue instead. WDYT?

@iliabylich
Copy link
Collaborator

ok, let's keep it as 2.8 for now. We can rename files whenever we want.

Could you split it into 2 PRs please?

@palkan palkan force-pushed the feat/endless-method branch 2 times, most recently from 04769a4 to ffc56ad Compare April 20, 2020 16:13
lib/parser/ruby28.y Outdated Show resolved Hide resolved
doc/AST_FORMAT.md Show resolved Hide resolved
Copy link
Collaborator

@iliabylich iliabylich left a comment

Choose a reason for hiding this comment

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

LGTM!

@@ -46,6 +46,7 @@ module Source
require 'parser/source/map/variable'
require 'parser/source/map/keyword'
require 'parser/source/map/definition'
require 'parser/source/map/endless_definition'
Copy link
Collaborator

Choose a reason for hiding this comment

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

I actually think that someday MRI developers may introduce endless classes/methods, so maybe we'll reuse it:

class A = def m = 42
module M = def m = 42

@iliabylich
Copy link
Collaborator

@palkan feel free to merge it

palkan added a commit to ruby-next/parser that referenced this pull request Apr 24, 2020
palkan added a commit to ruby-next/parser that referenced this pull request Apr 24, 2020
palkan added a commit to ruby-next/parser that referenced this pull request Apr 24, 2020
palkan added a commit to ruby-next/parser that referenced this pull request Apr 24, 2020
palkan added a commit to ruby-next/parser that referenced this pull request Apr 25, 2020
@palkan palkan mentioned this pull request Apr 25, 2020
palkan added a commit to ruby-next/parser that referenced this pull request Apr 25, 2020
@palkan palkan marked this pull request as ready for review April 29, 2020 18:49
@iliabylich iliabylich changed the title Endless method definition + ruby28.y: endless method definition Apr 29, 2020
@iliabylich iliabylich merged commit eb4a804 into whitequark:master Apr 29, 2020
@marcandre
Copy link
Contributor

So for me, of course those are semantic--Parser is intended, and was written for, tooling that requires those!

By semantic, I meant "related to the meaning of the code". It's in that sense that I don't consider the locations to be semantic. def foo = bar and def foo; bar; end, at least in my mind, mean exactly the same thing, even if the syntax used is different. There is no doubt that locations can be quite helpful and together with a semantic AST make parser a fabulous gem ❤️

@bbatsov I hope things are going better for you today 😄

So where are we at? Possibilities are:

  1. status quo
  2. emit def with nil location while allowing users to define def_endless_method to emit a different AST if they prefer that, which is + emit "endless method def" as :def node. #716
  3. introduce an emit_def_e or similar that outputs def or def_e, with a default that remains to be determined.

I'll be happy with 2 or 3 (with either defaults) and I'll definitely survive with 1.
@iliabylich: thanks for #716. Do you prefer this to option 3 above? @palkan what about you?

@iliabylich
Copy link
Collaborator

I'm not the best person to ask because I don't use parser (and if I'd use it starting from today there would be no difference). All 3 options are fine for me.

Also I'm actually slightly confused that new nodes cause exceptions. Parser::AST::Processor is the way to do traversing and it doesn't crash when there's an unknown node (ruby-parse -L uses it). rubocop, unparser and ruby-next - I thought all 3 easily handle new nodes by just ignoring them. rubocop can skip unknown nodes (and do no rewriting inside them), unparser/ruby-next can print node.loc.expression.source. At least it's better than runtime error IMO, but that's a completely separate topic to discuss. I was wrong initially as I thought that new nodes is the way to "save" other libraries from errors. Well, I guess I was wrong.

@palkan
Copy link
Contributor Author

palkan commented Jun 29, 2020

Although ruby-next already uses def_e, I don't think a lot of projects in the wild rely on edge syntax, so I can take care of the change in the next patch release.

I'm OK with any option, maybe, except from the 3rd one: introducing a flag is more like postponing the final decision.

palkan added a commit to ruby-next/parser that referenced this pull request Jul 2, 2020
palkan added a commit to ruby-next/parser that referenced this pull request Jul 7, 2020
palkan added a commit to ruby-next/parser that referenced this pull request Jul 24, 2020
palkan added a commit to ruby-next/parser that referenced this pull request Jul 29, 2020
palkan added a commit to ruby-next/parser that referenced this pull request Sep 1, 2020
palkan added a commit to ruby-next/parser that referenced this pull request Sep 2, 2020
palkan added a commit to ruby-next/parser that referenced this pull request Sep 2, 2020
palkan added a commit to ruby-next/parser that referenced this pull request Sep 8, 2020
palkan added a commit to ruby-next/parser that referenced this pull request Sep 28, 2020
palkan added a commit to ruby-next/parser that referenced this pull request Nov 17, 2020
palkan added a commit to ruby-next/parser that referenced this pull request Dec 23, 2020
palkan added a commit to ruby-next/parser that referenced this pull request Jun 4, 2021
palkan added a commit to ruby-next/parser that referenced this pull request Jun 4, 2021
palkan added a commit to ruby-next/parser that referenced this pull request Jun 4, 2021
palkan added a commit to ruby-next/parser that referenced this pull request Nov 28, 2021
palkan added a commit to ruby-next/parser that referenced this pull request Nov 30, 2021
palkan added a commit to ruby-next/parser that referenced this pull request Dec 28, 2021
palkan added a commit to ruby-next/parser that referenced this pull request Jan 10, 2022
palkan added a commit to ruby-next/parser that referenced this pull request Apr 5, 2022
palkan added a commit to ruby-next/parser that referenced this pull request Apr 5, 2022
palkan added a commit to ruby-next/parser that referenced this pull request Apr 5, 2022
palkan added a commit to ruby-next/parser that referenced this pull request Aug 2, 2022
palkan added a commit to ruby-next/parser that referenced this pull request Feb 27, 2023
palkan added a commit to ruby-next/parser that referenced this pull request Aug 29, 2023
palkan added a commit to ruby-next/parser that referenced this pull request Dec 4, 2023
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.

None yet

4 participants