jnthn / grammar-debugger

Grammar::Debugger and Grammer::Tracer Perl 6 modules
36 stars 20 forks source link

Grammar::Tracer fails to trace when parse is called on a grammar instance instead of a grammar class #4

Closed cognominal closed 10 years ago

cognominal commented 11 years ago

$ perl6

use Grammar::Tracer; grammar A {token TOP { a } }; A.new.parse('a') use Grammar::Tracer; grammar A {token TOP { a } }; A.new.parse('a') new | bless | | BUILDALL Cannot look up attributes in a type object

I don't think using grammar instances is specified but it works and makes a lot of sense because it allows to use instance variables.

I don't know if one can run two grammars (or grammar instances) at once thought

meisl commented 10 years ago

This is due to the current approach of testing for special method names like BUILD etc in order to NOT intercept them - which is rather brittle by construction: eg in case of grammar instances, a few more methods get looked up via find_method (called on the meta-class), such as new and bless, and those are simply missing from the "magic-names-list" as it is.

A better approach is to positively define which methods to intercept, once and for all. This would be any method of type Regex that's added and also parse, parsefile and subparse (those latter ones need to be intercepted differently and for a different reason). I've implemented this in pull-request #6, so that should fix the issue. (Still have to add tests for that; will do so soon - I'm trying to eliminate the code dupe between Tracer and Debugger)

cognominal commented 10 years ago

Yes, the current approach causes other problem as well. The find_method is not the proper way to intercept calls if one want to intercept regular calls to method of the grammars (as opposed to regexen) On the other hand, testing for Regex avoid to intercept regular method calls.

On 9/6/14, Matthias Kling notifications@github.com wrote:

This is due to the current approach of testing for special method names like BUILD etc for NOT intercepting them - which is rather brittle by construction: eg in case of using grammar instances a few more methods are looked up via find_method (called on the meta-class), such as new and bless, and those are simply missing from the "magic-names-list" as it is.

A better approach is to positively define which methods to intercept, once and for all. This would be any method of type Regex that's added and also parse, parsefile and subparse (those latter ones need to be intercepted differently and for a different reason). I've implemented this in pull-request #6, so that fixes this issue. (Still have to add tests for that but will do soon - I'm trying to eliminate the code dupe between Tracer and Debugger)


Reply to this email directly or view it on GitHub: https://github.com/jnthn/grammar-debugger/issues/4#issuecomment-54694032

cognominal stef

meisl commented 10 years ago

Well, to me it looks like find_method is never the right way to do intercepting, be it regexes or other methods. In order for it to work it's necessary to override publish_method_cache which effectively KILLs performance, near to unacceptable (for me, at least).

But I am definitely not a high-priest of the Higher Order Workings like eg @jnthn. I am, however, aware of the original scope of Grammar::Debugger, ie a demonstration piece. Nevertheless, it's already a very useful tool with quite a lot more potential IMHO, and I chose it for hacking on in hopes of a better understanding of the Higher Order Workings.

For example: isn't add_method the right place for such augmentations? But then, what about methods inherited from Grammar like parse (those don't show up there)? Also, I'm not quite able to use Routine.wrap for the task, which looks like having been made exactly for that. Instead I'm getting very weird errors "at a distance" which I don't understand. And there's a few more bits that puzzle me...

So, any input and/or advice, from high-priest or ordinary monk, will be highly appreciated :)

jnthn commented 10 years ago

I think instead of a name exclusion list, a better way to go could be checking that we actually have a Regex code object ($thingy-find-method-returns ~~ Regex). That way, there's no exclusion list needed, and we only pay attention to token/rule/regex, which is probably closer to what is desired.

Producing an updated method cache that pre-computes the closures would be good performance wise; it does complicate the code a bit, and yes, this was originally something I used to show off what was possible, so I was more focused on explainable code than raw speed. It has become by this point a rather useful tool though (heck, I used it to debug a $dayjob grammar earlier this week), and if trace overhead is a blocker for anyone then it's worth looking into the cache approach. It would be faster that way.

meisl commented 10 years ago

Well, I still think the existing method cache can be used, so there shouldn't be much added complication, if any at all. It is, however, inevitable to split up the code IMHO, as in pr #7. So you don't have it all in one file - which indeed may complicate showing off a bit ;)

But let's continue discussion in issue #10 "make it faster!"