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 commented Apr 16, 2020

TBD

@iliabylich
Copy link
Collaborator

iliabylich commented Apr 16, 2020

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

iliabylich commented Apr 16, 2020

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 ruby-next:feat/endless-method branch 2 times, most recently from 04769a4 to ffc56ad Apr 20, 2020
lib/parser/ruby28.y Outdated Show resolved Hide resolved
doc/AST_FORMAT.md Show resolved Hide resolved
@palkan palkan force-pushed the ruby-next:feat/endless-method branch from ffc56ad to f861723 Apr 22, 2020
@palkan palkan requested a review from iliabylich Apr 22, 2020
Copy link
Collaborator

iliabylich left a comment

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'

This comment has been minimized.

Copy link
@iliabylich

iliabylich Apr 22, 2020

Collaborator

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

iliabylich commented Apr 22, 2020

@palkan feel free to merge it

palkan added a commit to ruby-next/parser that referenced this pull request Apr 24, 2020
Port of whitequark#676
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 palkan force-pushed the ruby-next:feat/endless-method branch from f861723 to 67f58c0 Apr 25, 2020
palkan added a commit to ruby-next/parser that referenced this pull request Apr 25, 2020
@palkan palkan force-pushed the ruby-next:feat/endless-method branch from 67f58c0 to c839a26 Apr 25, 2020
@palkan palkan mentioned this pull request Apr 25, 2020
@palkan palkan force-pushed the ruby-next:feat/endless-method branch from c839a26 to 94f65c2 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 Apr 29, 2020
@palkan palkan force-pushed the ruby-next:feat/endless-method branch from 94f65c2 to 190c50f Apr 29, 2020
@iliabylich iliabylich changed the title Endless method definition + ruby28.y: endless method definition Apr 29, 2020
@marcandre
Copy link
Contributor

marcandre commented Jun 25, 2020

Well, maybe it's not a direct explanation, but I think that just AST (without source maps) should be enough to construct the same source code.

If by "same" you mean character by character, then I completely disagree with you and you clearly can't do it for the majority of nodes; good luck with ? :, unless, if, but more simply just with def: use parenthesis or not for the argument list? how about spacing? This distinction is in the readme of https://github.com/mbj/unparser gem.

If by "same" you mean that they produce exact replica of the code that will always behave entirely the same way under all circumstances, then I completely agree with you. That is exactly my point actually. I challenge you to find an example where I won't be able to produce equivalent code (without checking the actual source or location map) if def_e were emitted as def. All def_e can be written as 100% equivalent def and vice versa. unparser does not need a def_e. It's 100% pure syntactic sugar and therefore should not be treated as a different node type.

@iliabylich
Copy link
Collaborator

iliabylich commented Jun 25, 2020

You are right, all def_e nodes could be emitted as def nodes.

I didn't mean that current AST format is the-most-optimal and there are no nodes that could be replaced with equivalents. Of course we can get rid of things like for loop (by replacing it with .each {} just like MRI does under the hood) and case statement (by replacing it with a chain of ifs with ===). I guess my initial explanation was wrong from this POV. AST is definitely not only about un-parsing. I think it's important to give information about all kinds of nodes if possible, but you right, it's unclear when it should be a new node and when it's OK to just change source maps (if there's no difference between them from runtime perspective).

It's hard to make this decision (if possible at all) because it's completely unclear (at least for me) how Ruby grammar is going to change. Will there be any grammar features available only for one-line methods? Ruby has no RFCs and we have to put some assumptions like the one that was made in this PR.

What should we do if such new syntax will be introduce some day? Another compatibility flag? I don't like them and I don't want them to grow every time we change the format. For me it's a trade off between keeping AST format way too simple and adding flags every time we want to change it vs keeping it big and verbose, but forcing users to support more node types.

@marcandre
Copy link
Contributor

marcandre commented Jun 25, 2020

Of course we can get rid of things like for loop (by replacing it with .each {} just like MRI does under the hood)

I believe this is incorrect. Scoping is different. for i in 1..10; end; p i vs (1..10).each { |i| }; p i are different

and case statement (by replacing it with a chain of ifs with ===).

Also incorrect (at least with case expr). You would have to create a local variable to hold the expr, which has a detectable impact on binding and would hold a reference to the object a different length of time.

I wrote "always behave entirely the same way under all circumstances" and I meant it. If they're the same under 99.99% of the circumstances, they are not entirely the same.

I guess my initial explanation was wrong from this POV.

I think your initial explanation was actually completely correct, you shouldn't discount your (actually correct) intuitive understanding because it doesn't yield the (actually incorrect) result you think it should 😄

What should we do if such new syntax will be introduce some day? Another compatibility flag?

I agree completely that compatibility flags should be minimized.

Abstractly, parser is a set of mappings from all possible Ruby code to equivalent ASTs, one mapping for each supported Ruby versions.

To me, compatibility means that for all for any subset of those mappings, ruby code that are equivalent across those Ruby versions should have the same corresponding AST and that this AST never changes. I'm excluding ruby-head (2.8 here) as it can't offer compatibility until 2.8 is actually released.

If my Ruby 2.6 code produced a particular AST with parser, I expect that AST to not change (except for bug fixes). I also expect to get the same AST if using Parser::Ruby27 and all future versions (assuming that the particular ruby code is still interpreted by Ruby the same way)

When support for Ruby 2.8 is added, running with Parser::Ruby28 on the same 2.6-compatible code should produce the same AST. The possible resulting ASTs from Parser::Ruby28, when considering Ruby 2.6 compatible code, are the same as those of Parser::Ruby26 and Ruby27. But running it on 2.8 code that is not equivalent or compatible with Ruby 2.6/2.7 should have no compatibility guarantee besides being an AST::Node.

It's clear to me that syntax changes in Ruby will sometimes change just the location (as it should here), sometimes introduce different node types and sometimes change expectations on children. As long as that doesn't impact older Ruby code, that is fine.

New node types should be reserved when the Ruby construct is actually introducing semantically new concepts (e.g. ... which can't quite completely be replaced by other forwarding mechanisms, sadly).

koic added a commit to koic/rubocop-ast that referenced this pull request Jun 26, 2020
This PR supports "Endless method definition" syntax for Ruby 2.8 (3.0).
Parser gem supports this syntax by whitequark/parser#676.

Ref: https://bugs.ruby-lang.org/issues/16746
koic added a commit to koic/rubocop-ast that referenced this pull request Jun 26, 2020
This PR supports "Endless method definition" syntax for Ruby 2.8 (3.0).
Parser gem supports this syntax by whitequark/parser#676.

Ref: https://bugs.ruby-lang.org/issues/16746
@whitequark
Copy link
Owner

whitequark commented Jun 26, 2020

If I recall correctly, then a single node type (like def) always has the same source map subclass, so that you can check which non-semantic variant you got by looking at which fields are nil?, not by checking is_a? Parser::Source::Map::Whatever which is an implementation detail. So if you introduce a new source map subclass with different fields it's a breaking change for the downstream tooling.

Another way to think about it is: suppose I write a tool that looks at the ends of methods. It expects every def to have one. If the tool silently breaks with a NoMethodError somewhere in its guts with updated Parser then this is of course the fault of parser and a violation of its backwards compatibility promise. If it breaks on an unknown node type then that's expected behavior. So I think @iliabylich did the right thing here.

@marcandre

Abstractly, parser is a set of mappings from all possible Ruby code to equivalent ASTs, one mapping for each supported Ruby versions.

I didn't use any absolute statement like that as guidance when developing Parser. What I considered was, is it actually convenient for downstream tooling? Does it create cases where downstream tooling would have to do weird things to disambiguate? It shouldn't. AFAICT folding def_e into def would require downstream tooling to do weird things so I think it's a bad idea to do so.

koic added a commit to koic/rubocop-ast that referenced this pull request Jun 26, 2020
This PR supports "Endless method definition" syntax for Ruby 2.8 (3.0).
Parser gem supports this syntax by whitequark/parser#676.

Ref: https://bugs.ruby-lang.org/issues/16746
@marcandre
Copy link
Contributor

marcandre commented Jun 26, 2020

Thanks for the reply.

Looks like I'm not convincing anybody (and I'm not getting convinced). That's ok 😄

I have few more questions below, feel free to answer only those that you find interesting (if any). I've simplified by ignoring defs / defs_e.

  1. We keep talking "backward compatiblity", but it's really "forward compatibility", right?

  2. The only reason I hear is that def_e is for compatiblity. So if this was the first ever release of parser, or if that syntax had been in Ruby since 1.8, do we agree that it would only emit def? So parser will emit "less than optimal" AST for compatibility? What are the other examples where parser does not emit the "optimal" AST for forward compatibility reasons?

  3. @whitequark: do you regret having a single node type for if, unless and ?:? I recall finding it very surprising. Now I find it ingenious.

  4. If Ruby had introduced %w[string other_string] only today, would we create a new str_something to avoid having a str with no begin and end locations?

  5. Why is having irange / erange accepting a nil child (endless/beginless ranges) considered more forward compatible than having a nil end location? (For the record, I completely agree it was the right decision, but it seems like a less compatible change to me)

  6. I probably should have started with this, but as far as compatiblity goes, introducing a def_e crashes all existing versions of RuboCop (because Commissionner expects knowing all node types, but that could be remedied). Even if that was/had been remedied (for example that Commissionner forwarded unknown calls to cops that respond to them, as it probably should), it would still break all cops relying on VariableForce (which analyses scopes of variables for them), all cops implementing on_def (and not on_def_e), all NodePatterns with a "def" in there, and crash the gem unparser (I imagine) and DeepCover (definitely).

By contrast, with a def with no end location, RuboCop works (all versions), VariableForce works, most cops implementing on_def too (*), all NodePatterns with a "def" work, the gem unparser and DeepCover work out of the box.

(*) I haven't verified which builtin cops rely on location.end for def nodes, but my assumption is that few do. on_def is pretty important as a scope, but I presume that the end location is important to very few cops; Clearly the Cop that checks the indentation of def vs end will break :-)
Few cops will need to rewrite the whole def, or insert things begfore/after the end, and since expression is still valid they might be relying on that.

Which known tools will work out of the box as designed with def_e? Any of those wouldn't work with def with no end location?

@whitequark
Copy link
Owner

whitequark commented Jun 26, 2020

1. We keep talking "backward compatiblity", but it's really "forward compatibility", right?

I think what you call it depends on the point of view. I'd probably call it "AST stability" if I tried to be more clear.

2. The only reason I hear is that def_e is for compatiblity. So if this was the first ever release of parser, or if that syntax had been in Ruby since 1.8, do we agree that it would only emit def? So parser will emit "less than optimal" AST for compatibility?

Correct.

3. @whitequark: do you regret having a single node type for if, unless and ?:? I recall finding it very surprising. Now I find it ingenious.

Nope, it was a deliberate design decision. Parser doesn't normalize everything (there's kwbegin for example) but it normalizes a great deal to make life easier for AST consumers.

4. If Ruby had introduced %w[string other_string] only today, would we create a new str_something to avoid having a str with no begin and end locations?

Probably, yeah.

5. Why is having irange / erange accepting a nil child (endless/beginless ranges) considered more forward compatible than having a nil end location?

I'm no longer involved in day-to-day decisions, so ask @iliabylich. Arguably the decisions here should have been the same.

6. I probably should have started with this, but as far as compatiblity goes, introducing a def_e crashes all existing versions of RuboCop

Okay, so adding def_e requires downstream tooling to do weird things after all. This means I was mistaken. I will follow my own principle and ask @iliabylich to revert this change and implement @marcandre's suggestion.

@iliabylich
Copy link
Collaborator

iliabylich commented Jun 26, 2020

@whitequark Now it's my turn to say "I don't follow".

Let's imagine that def_e is implemented as def with empty end location. Are there any guarantees that libraries that use parser implement all required support (i.e. they add if checks in appropriate places) before the next version of parser is released? That's great if so.

But what if there's no support? End users (of rubocop, let's say) start using def foo: value syntax and get errors.

On the other side, if it's implemented as a new def_e node it would be simply ignored (because there's no code that goes into these nodes). Which one is better? I always thought it's the latter.

Also, it makes old versions of rubocop incompatible with new parser, i.e. if users do bundle update parser the get same errors coming from the new syntax.

IMO endless (and beginningless) ranges should've been implemented as new nodes too, I think it was a mistake from me.

Who is the main consumer? End users or 3rd party libraries that use parser?

@marcandre
Copy link
Contributor

marcandre commented Jun 26, 2020

Let's imagine that def_e is implemented as def with empty end location. Are there any guarantees that libraries that use parser implement all required support (i.e. they add if checks in appropriate places) before the next version of parser is released? That's great if so.

For many cases, there are no checks to make because there's rarely a need to check all the locations. What is that library doing that requires it to use the end location? Is there an example?

But what if there's no support? End users (of rubocop, let's say) start using def foo: value syntax and get errors.

I'm not sure I understand. As I explained end users of current RuboCop will systematically get a crash with def_e even if running a single Cop (although this could be mitigated)

On the other side, if it's implemented as a new def_e node it would be simply ignored (because there's no code that goes into these nodes).

Currently this is not true for RuboCop, will never be true for DeepCover, and I presume cannot be true for unparser.

Also, it makes old versions of rubocop incompatible with new parser, i.e. if users do bundle update parser the get same errors coming from the new syntax.

With no def_e, it will be only for some cops (which can be disabled), and RuboCop provides nice error messages in those cases. With def_e you get a crash.

IMO endless (and beginningless) ranges should've been implemented as new nodes too, I think it was a mistake from me.

As new nodes they would also have crashed RuboCop (but that could be mitigated).

As far as RuboCop is concerned, I'm 100% ok with RuboCop not running on previously unknown versions of Ruby. My main concern is to minimize the work involved with the compatibility of the very many cops existing (builtin or not). For this, I feel that an AST that is sticks close to our intuitive understanding is best. That's why I consider the AST (type + children = "what is it") and location to be very secondary ("where/how is it written").

@palkan
Copy link
Contributor Author

palkan commented Jun 27, 2020

Hey, everyone!

I'm a bit late but as the original initiator and the author of this feature, I feel that I should jump into a discussion.

I also proposed using the absence of the end location as an endless method definition indicator; @iliabylich provided me with the compatibility argument and I agreed with him. And I still agree.

One thing that haven't been mentioned during the recent discussion, is another difference between def and def_e: endless def-s also have an assignment "operator" (=), and we have the corresponding assignment location in the new source map class. And endless method is syntactically different to a regular one not only due to the absence of the end keyword but by the presence of = which is not possible for regular methods.

introducing a def_e crashes all existing versions of RuboCop (because Commissionner expects knowing all node types, but that could be remedied)

IMO, that should be fixed at the RuboCop's side.

Generally speaking, I think, crashes due to new node types is a downstream software problem. New nodes are added regularly (especially in the recent years, Ruby syntax is evolving fast). Downstream software should be prepared for that, it should be able to handle new nodes gracefully.

On the other hand, when changing the existing nodes breaks downstream tools—that's more a Parser problem. For example, the already mentioned re-usage of irange / erange for endless ranges broke Unparser: mbj/unparser#137 (adding a new node would also result in a crash but with a more clear exception: unsupported node type).

Anyway, I like the idea that @whitequark described:

is it actually convenient for downstream tooling? Does it create cases where downstream tooling would have to do weird things to disambiguate?

That led me to the following idea: what if we come up with some kind of an informal (or even formal) process of modifying AST_FORMAT.md? For example, we can request reviews from major downstream software maintainers (to have a discussion like this before the feature is merged).

An alternative idea is to provide canary/edge/whatever releases with the features for the edge Ruby versions. That would make it possible to update the format without worrying about the compatibility while it's not a part of a stable release.

@whitequark
Copy link
Owner

whitequark commented Jun 27, 2020

I have another proposal that would hopefully be a good compromise between the two we have now: having downstream clients opt into node types they support. For example, if old Rubocop doesn't support def_e, it would not opt into getting it, and then Parser itself would emit a nice diagnostic instead of emiting a def_e node and crashing the downstream application.

@whitequark
Copy link
Owner

whitequark commented Jun 27, 2020

For that matter you don't have to make the opt-in at node type granularity. You could opt into getting ranges with nils in them just as well.

@marcandre
Copy link
Contributor

marcandre commented Jun 27, 2020

On the other hand, when changing the existing nodes breaks downstream tools—that's more a Parser problem. For example, the already mentioned re-usage of irange / erange for endless ranges broke Unparser: mbj/unparser#137 (adding a new node would also result in a crash but with a more clear exception: unsupported node type).

My opinion is that we should not compromise on the AST we produce so that tools can try to be forward compatible. For endless node ranges, unparser broke. There was no other alternative anyways. Maybe my opinion is not shared, but that is not a problem.

FWIW, DeepCover broke too mainly because we are super strict on the AST we process. Every single AST type has to be explicitly vetted, all its arguments (single or variable) is named and the possible types of these arguments is also specified:

class Range
-  has_child from: Node
-  has_child to: Node
+  has_child from: [Node, nil]
+  has_child to: [Node, nil]

OTOH, DeepCover makes as few assumptions on location as possible. I originally thought that location was a kind of OpenStruct. Maybe I have the wrong mental image.

That led me to the following idea: what if we come up with some kind of an informal (or even formal) process of modifying AST_FORMAT.md?

I'm very impressed by @iliabylich's dilligence in implementing all the syntax changes, and I should be more attentive. I'm fine to be pinged for these changes, but I am expecting to agree with @iliabylich the vast majority of times and I wouldn't want @iliabylich to have to wait for a lot of approvals, as long as there is openness to revisit a decision like in this case, in the months before the new syntax changes are released at X-mas.

@marcandre
Copy link
Contributor

marcandre commented Jun 27, 2020

I have another proposal that would hopefully be a good compromise between the two we have now: having downstream clients opt into node types they support.

This seems like a lot of work for most clients (trivial for DeepCover, unless you want to include the locations in there in which case a lot of work) and a super complex API too.

If you want to go this way, downstream clients could simply specify which Ruby version they are aware of. There's already a crude way for them to check if they are a bit out of their depth though, which is to not use parser/current 😅, or checking the result against a set of known parsers...

@whitequark
Copy link
Owner

whitequark commented Jun 27, 2020

This seems like a lot of work for most clients (trivial for DeepCover, unless you want to include the locations in there in which case a lot of work) and a super complex API too.

If you reject that, the question becomes: do we break the clients with (more) new nodes, or do we break the clients with (more) changes to existing nodes?

@marcandre
Copy link
Contributor

marcandre commented Jun 27, 2020

If you reject that

I don't quite reject it. I say that the list of nodes a client is opting-in, with the possible children and locations doesn't need to be spelled out in full as it corresponds exactly to a given Ruby version.

As far as I'm concerned, a tool that is unaware of Parser26 and uses it nevertheless might break when using it on code that Parser25 could not process. The tool is free to use Parser25 (in which case parser will generate a nice syntax error on code using the new syntax), or try with Parser26 and maybe it'll work, but probably not.

do we break the clients with (more) new nodes, or do we break the clients with (more) changes to existing nodes?

My opinion is that the changes should reflect in the best way the actual Ruby syntax change. The closer we are to the semantics of the new Ruby code, the less difficult it will be for the clients to adapt, since what they are trying to do is directly related to what the code means.

As a consequence, changes that affect only the locations are not semantic and should be preferred to changes of AST children / types (like here).

I sense that there might be a feeling that introducing a new type is the best way to avoid incompatibilities or is to be more expected than other changes but I believe it is the contrary. It doesn't actually help. Unless it's a different concept, there should not be a different AST type. Having irange with a nil child was the right decision because of that; it's not a new concept deserving of a new node type. Forward args (sadly) is a new concept and it is definitely out of the question to use restarg and say check the source 😅

@whitequark
Copy link
Owner

whitequark commented Jun 27, 2020

I say that the list of nodes a client is opting-in, with the possible children and locations doesn't need to be spelled out in full

I wasn't talking about that. I was talking about a mechanism similar to the existing one we have, e.g. emit_def_e like emit_forward_arg.

As a consequence, changes that affect only the locations are not semantic and should be preferred to changes of AST children / types (like here).

Changes that affect locations absolutely are semantic, since the whole fucking point of me developing Parser in first place is to provide source locations. If you don't care about locations just use Ripper.

@marcandre
Copy link
Contributor

marcandre commented Jun 27, 2020

I wasn't talking about that. I was talking about a mechanism similar to the existing one we have, e.g. emit_def_e like emit_forward_arg.

Oh, I stated before that I'm fine with emit_def_e if it's deemed the best solution.

the whole fucking point of me developing Parser in first place is to provide source locations.

I hope your use of language isn't a sign of exasperation. I only wish for an interesting discussion and the best parser gem possible. I will respect your decision and @iliabylich's, and even if it includes deciding to take a break or close the discussion if it becomes tedious to either of you.

If you don't care about locations just use Ripper

I imagine you know, but in case you don't, Ripper produces a particularly horrible, unusable, unsemantic and even broken AST:

foo(:bar) vs foo :bar => different Sexp
%i[foo] vs [:foo] => different Sexp
%i[foo] vs %w[foo] => exactly the same Sexp!?!

It's actually not an AST but a parse tree, and a broken one at that...

@whitequark
Copy link
Owner

whitequark commented Jun 27, 2020

I hope your use of language isn't a sign of exasperation.

Sorry, I had an awful day. It doesn't have much to do with this thread, and I should have just replied later.

Let me rephrase. The reason I started Parser when ruby_parser and Ripper existed was to have precise source locations. So for me, of course those are semantic--Parser is intended, and was written for, tooling that requires those!

@marcandre
Copy link
Contributor

marcandre commented Jun 28, 2020

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 #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

iliabylich commented Jun 28, 2020

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.