Re: Doc Comments cause parser errors

Sascha Schumann

23 years ago

On Wed, 2 Apr 2003, Andi Gutmans wrote:

> AFAIK, doc comments are for classes and functions only. I think this is
> what's important because interfaces are functions not variables.

Hm, so my PHP 4-stylish code would cause a parse error, if it
contained a valid character like a "*" after the initiation
sequence "/*"?

- Sascha

Derick Rethans

23 years ago

On Wed, 2 Apr 2003, Sascha Schumann wrote:

> On Wed, 2 Apr 2003, Andi Gutmans wrote:
>
> > AFAIK, doc comments are for classes and functions only. I think this is
> > what's important because interfaces are functions not variables.
>
>     Hm, so my PHP 4-stylish code would cause a parse error, if it
>     contained a valid character like a "*" after the initiation
>     sequence "/*"?

Sounds indeed like something we don't want, as almost all the
documentors use this.

Derick

--
"my other box is your windows PC"
-------------------------------------------------------------------------
Derick Rethans                                 http://derickrethans.nl/
PHP Magazine - PHP Magazine for Professionals       http://php-mag.net/
-------------------------------------------------------------------------

Andi Gutmans

23 years ago

At 08:16 AM 4/2/2003 +0200, Sascha Schumann wrote:

>On Wed, 2 Apr 2003, Andi Gutmans wrote:
>
> > AFAIK, doc comments are for classes and functions only. I think this is
> > what's important because interfaces are functions not variables.
>
>     Hm, so my PHP 4-stylish code would cause a parse error, if it
>     contained a valid character like a "*" after the initiation
>     sequence "/*"?

Oh, I didn't think of that. You are completely right. This is definitely a
problem. Andrei's patch returns a token for /** */ on the parser level so
only places which are allowed to have this work.
This sounds like a serious flaw because any place which has such comments
will fail. I guess we'll need to revert his patch and try to think of a
completely different way of doing it (unless someone has a good idea of how
to solve this problem).

Andi

Andrei Zmievski

23 years ago

On Wed, 02 Apr 2003, Andi Gutmans wrote:

> Oh, I didn't think of that. You are completely right. This is definitely a
> problem. Andrei's patch returns a token for /** */ on the parser level so
> only places which are allowed to have this work.
> This sounds like a serious flaw because any place which has such comments
> will fail. I guess we'll need to revert his patch and try to think of a
> completely different way of doing it (unless someone has a good idea of how
> to solve this problem).

No, it won't fail. Look:

statement:
unticked_statement { zend_do_ticks(TSRMLS_C); }
|   doc_comment { zend_do_free(&$1 TSRMLS_CC); } unticked_statement { zend_do_ticks(TSRMLS_C); }
;

This allows doc comments before any non-declarative statement. The only
remaining problem is having doc comments before class vars and
constants.

If you think it's better to go with the approach that Alan outlined, I
can live with that too, but it seems easier to handle this in the parser
rather than in zend_do_* functions and also having to reset the doc
comment for each statement.

-Andrei                                       http://www.gravitonic.com/

The Feynman problem solving algorithm:

1) Write down the problem.
2) Think real hard.
3) Write down the answer.

Andi Gutmans

23 years ago

At 10:41 AM 4/2/2003 -0500, Andrei Zmievski wrote:

>On Wed, 02 Apr 2003, Andi Gutmans wrote:
> > Oh, I didn't think of that. You are completely right. This is definitely a
> > problem. Andrei's patch returns a token for /** */ on the parser level so
> > only places which are allowed to have this work.
> > This sounds like a serious flaw because any place which has such comments
> > will fail. I guess we'll need to revert his patch and try to think of a
> > completely different way of doing it (unless someone has a good idea of
> how
> > to solve this problem).
>
>No, it won't fail. Look:
>
>  statement:
>         unticked_statement { zend_do_ticks(TSRMLS_C); }
>     |   doc_comment { zend_do_free(&$1 TSRMLS_CC); } unticked_statement {
> zend_do_ticks(TSRMLS_C); }
>  ;
>
>This allows doc comments before any non-declarative statement. The only
>remaining problem is having doc comments before class vars and
>constants.
>
>If you think it's better to go with the approach that Alan outlined, I
>can live with that too, but it seems easier to handle this in the parser
>rather than in zend_do_* functions and also having to reset the doc
>comment for each statement.

How about:
if ($a
/**
$a is great variable */
|| $b) {

}

I'm sure I can find other more realistic examples which would break. I
don't know why I didn't think of it before but I think having this in the
parser is impossible. We need to find a smarter way of doing it.

Andi

Andrei Zmievski

23 years ago

On Wed, 02 Apr 2003, Andi Gutmans wrote:

> How about:
> if ($a
> /**
> $a is great variable */
>         || $b) {
>
> }
>
> I'm sure I can find other more realistic examples which would break. I
> don't know why I didn't think of it before but I think having this in the
> parser is impossible. We need to find a smarter way of doing it.

Suggestions?

-Andrei                                       http://www.gravitonic.com/
* All of the above is my opinion, unless specified otherwise. *

Zeev Suraski

23 years ago

At 09:23 02/04/2003, Andi Gutmans wrote:

>At 08:16 AM 4/2/2003 +0200, Sascha Schumann wrote:
>>On Wed, 2 Apr 2003, Andi Gutmans wrote:
>>
>> > AFAIK, doc comments are for classes and functions only. I think this is
>> > what's important because interfaces are functions not variables.
>>
>>     Hm, so my PHP 4-stylish code would cause a parse error, if it
>>     contained a valid character like a "*" after the initiation
>>     sequence "/*"?
>
>Oh, I didn't think of that. You are completely right. This is definitely a
>problem. Andrei's patch returns a token for /** */ on the parser level so
>only places which are allowed to have this work.
>This sounds like a serious flaw because any place which has such comments
>will fail. I guess we'll need to revert his patch and try to think of a
>completely different way of doing it (unless someone has a good idea of
>how to solve this problem).

Why not just add the doc comment rule to variables as well, and silently
ignore it?  (I didn't check whether it can be done easily, but I imagine it
can be done...).

Zeev

Andi Gutmans

23 years ago

At 10:13 AM 4/2/2003 +0300, Zeev Suraski wrote:

>At 09:23 02/04/2003, Andi Gutmans wrote:
>>At 08:16 AM 4/2/2003 +0200, Sascha Schumann wrote:
>>>On Wed, 2 Apr 2003, Andi Gutmans wrote:
>>>
>>> > AFAIK, doc comments are for classes and functions only. I think this is
>>> > what's important because interfaces are functions not variables.
>>>
>>>     Hm, so my PHP 4-stylish code would cause a parse error, if it
>>>     contained a valid character like a "*" after the initiation
>>>     sequence "/*"?
>>
>>Oh, I didn't think of that. You are completely right. This is definitely
>>a problem. Andrei's patch returns a token for /** */ on the parser level
>>so only places which are allowed to have this work.
>>This sounds like a serious flaw because any place which has such comments
>>will fail. I guess we'll need to revert his patch and try to think of a
>>completely different way of doing it (unless someone has a good idea of
>>how to solve this problem).
>
>Why not just add the doc comment rule to variables as well, and silently
>ignore it?  (I didn't check whether it can be done easily, but I imagine
>it can be done...).

This problem bites just about any places where people used /** */ as comments.
For example:
if ($a > 5) { /** This is a smart check *.
}

IMO, we can't break BC this badly.

Andi

Zeev Suraski

23 years ago

At 10:17 02/04/2003, Andi Gutmans wrote:

>At 10:13 AM 4/2/2003 +0300, Zeev Suraski wrote:
>>At 09:23 02/04/2003, Andi Gutmans wrote:
>>>At 08:16 AM 4/2/2003 +0200, Sascha Schumann wrote:
>>>>On Wed, 2 Apr 2003, Andi Gutmans wrote:
>>>>
>>>> > AFAIK, doc comments are for classes and functions only. I think this is
>>>> > what's important because interfaces are functions not variables.
>>>>
>>>>     Hm, so my PHP 4-stylish code would cause a parse error, if it
>>>>     contained a valid character like a "*" after the initiation
>>>>     sequence "/*"?
>>>
>>>Oh, I didn't think of that. You are completely right. This is definitely
>>>a problem. Andrei's patch returns a token for /** */ on the parser level
>>>so only places which are allowed to have this work.
>>>This sounds like a serious flaw because any place which has such
>>>comments will fail. I guess we'll need to revert his patch and try to
>>>think of a completely different way of doing it (unless someone has a
>>>good idea of how to solve this problem).
>>
>>Why not just add the doc comment rule to variables as well, and silently
>>ignore it?  (I didn't check whether it can be done easily, but I imagine
>>it can be done...).
>
>This problem bites just about any places where people used /** */ as comments.
>For example:
>if ($a > 5) { /** This is a smart check *.
>}
>
>IMO, we can't break BC this badly.

Ah, that's right.  We probably need to rethink our strategy then, and tie
doc comments to the relevant tokens that follow them.  Not a very easy task
I think.

Zeev

Alan Knowles

23 years ago

http://cvs.php.net/diff.php/ZendEngine2/zend_compile.c?r1=1.403&r2=1.404&ty=h&num=10

removed the T_DOC_COMMENT from being ignored by zendlex -

looks like at the moment, optional_doc_comment is splattered all over
the parser.

the other way to do this (is what phpcodedoc does) is just to
remembering the last comment block (pointer to startpos, and length
within the compiled string.) - leave the parser ignoring T_DOC_COMMENT tags.

- reset the startpos to -1 it at the end of each statement.  (comments
in the wrong place are just ignored)

then copying it when you declare classes/vars etc. directly from
startpos/length.. (and reset the startpos).

putting in the parser is just a bit too messy.. IMO :)

Regards
Alan

>> This problem bites just about any places where people used /** */ as
>> comments.
>> For example:
>> if ($a > 5) { /** This is a smart check *.
>> }
>>
>> IMO, we can't break BC this badly.
>
>
> Ah, that's right.  We probably need to rethink our strategy then, and
> tie doc comments to the relevant tokens that follow them.  Not a very
> easy task I think.
>
> Zeev
>
>

--
Can you help out?
Need Consulting Services or Know of a Job?
http://www.akbkhome.com

Andrei Zmievski

23 years ago

On Wed, 02 Apr 2003, Alan Knowles wrote:

> http://cvs.php.net/diff.php/ZendEngine2/zend_compile.c?r1=1.403&r2=1.404&ty=h&num=10
>
> removed the T_DOC_COMMENT from being ignored by zendlex -
>
> looks like at the moment, optional_doc_comment is splattered all over
> the parser.
>
> the other way to do this (is what phpcodedoc does) is just to
> remembering the last comment block (pointer to startpos, and length
> within the compiled string.) - leave the parser ignoring T_DOC_COMMENT tags.
>
> - reset the startpos to -1 it at the end of each statement.  (comments
> in the wrong place are just ignored)
>
> then copying it when you declare classes/vars etc. directly from
> startpos/length.. (and reset the startpos).
>
> putting in the parser is just a bit too messy.. IMO :)

All right. I'll try something like the above approach.

-Andrei                                       http://www.gravitonic.com/
* Ethernet n.: something used to catch the etherbunny. *

Andrei Zmievski

23 years ago

> >This problem bites just about any places where people used /** */ as
> >comments.
> >For example:
> >if ($a > 5) { /** This is a smart check *.
> >}
> >
> >IMO, we can't break BC this badly.
>
> Ah, that's right.  We probably need to rethink our strategy then, and tie
> doc comments to the relevant tokens that follow them.  Not a very easy task
> I think.

Actually the doc comment has to start with "/**\n" and end with "*/", so
the above will work just fine (because I have a rule for doc comments
with unticked_statement). The only issue right now is making rules for
doc comments before class/namespace vars and constants. If I could get
some help with that from Zeev/Andi/Stas, it'd be great.

-Andrei                                       http://www.gravitonic.com/

A room without books is like a body without a soul.
-- Marcus Tullius Cicero (106-43 B.C.)

Andi Gutmans

23 years ago

At 10:38 AM 4/2/2003 -0500, Andrei Zmievski wrote:

> > >This problem bites just about any places where people used /** */ as
> > >comments.
> > >For example:
> > >if ($a > 5) { /** This is a smart check *.
> > >}
> > >
> > >IMO, we can't break BC this badly.
> >
> > Ah, that's right.  We probably need to rethink our strategy then, and tie
> > doc comments to the relevant tokens that follow them.  Not a very easy
> task
> > I think.
>
>Actually the doc comment has to start with "/**\n" and end with "*/", so
>the above will work just fine (because I have a rule for doc comments
>with unticked_statement). The only issue right now is making rules for
>doc comments before class/namespace vars and constants. If I could get
>some help with that from Zeev/Andi/Stas, it'd be great.

No matter how you define it (with \n or without) I think it can't be in the
parser but has to be caught on the scanner level.

Andi

Zeev Suraski

23 years ago

At 18:38 02/04/2003, Andrei Zmievski wrote:

> > >This problem bites just about any places where people used /** */ as
> > >comments.
> > >For example:
> > >if ($a > 5) { /** This is a smart check *.
> > >}
> > >
> > >IMO, we can't break BC this badly.
> >
> > Ah, that's right.  We probably need to rethink our strategy then, and tie
> > doc comments to the relevant tokens that follow them.  Not a very easy
> task
> > I think.
>
>Actually the doc comment has to start with "/**\n" and end with "*/", so
>the above will work just fine (because I have a rule for doc comments
>with unticked_statement). The only issue right now is making rules for
>doc comments before class/namespace vars and constants. If I could get
>some help with that from Zeev/Andi/Stas, it'd be great.

I don't really like the idea of cluttering the parser with additional rules
if it's not really necessary.  I think that the idea proposed by Alan is
actually the best way to go (keep the last comment in a global, use and
reset it in the next relevant declaration implementation).  IIRC, that's
also the way the Zend Development Environment does the job :)

Zeev

Andrei Zmievski

23 years ago

On Wed, 02 Apr 2003, Zeev Suraski wrote:

> I don't really like the idea of cluttering the parser with additional rules
> if it's not really necessary.  I think that the idea proposed by Alan is
> actually the best way to go (keep the last comment in a global, use and
> reset it in the next relevant declaration implementation).  IIRC, that's
> also the way the Zend Development Environment does the job :)

What about this case:

$a = 5;
/**
* set $b to 3
*/
$b = 3;

class Bar {
}

We don't want to apply this doc comment to class Bar, so it needs to be
reset on every statement, basically. Isn't that right?

-Andrei                                       http://www.gravitonic.com/
* Anything will fit if you push hard enough. *

Zeev Suraski

23 years ago

At 18:51 02/04/2003, Andrei Zmievski wrote:

>On Wed, 02 Apr 2003, Zeev Suraski wrote:
> > I don't really like the idea of cluttering the parser with additional
> rules
> > if it's not really necessary.  I think that the idea proposed by Alan is
> > actually the best way to go (keep the last comment in a global, use and
> > reset it in the next relevant declaration implementation).  IIRC, that's
> > also the way the Zend Development Environment does the job :)
>
>What about this case:
>
>         $a = 5;
>         /**
>         * set $b to 3
>         */
>         $b = 3;
>
>         class Bar {
>         }
>
>We don't want to apply this doc comment to class Bar, so it needs to be
>reset on every statement, basically. Isn't that right?

Probably, yes.  I think Alan mentioned it.

Zeev

Andrei Zmievski

23 years ago

On Wed, 02 Apr 2003, Zeev Suraski wrote:

> >We don't want to apply this doc comment to class Bar, so it needs to be
> >reset on every statement, basically. Isn't that right?
>
> Probably, yes.  I think Alan mentioned it.

So it would be enough to reset it in unticked_statement rule?

-Andrei                                       http://www.gravitonic.com/

"Music expresses that which can not be said
and on which it is impossible to be silent."
-Victor Hugo

Lance Lovette

23 years ago

Can the purpose of doc comments be clarified? Why aren't class variables
important enough to document? I think everything exposed by a class -
variables, methods, constants, etc. - are all equally important and should
be documented.

Andrei Zmievski

23 years ago

On Wed, 02 Apr 2003, Lance Lovette wrote:

> Can the purpose of doc comments be clarified? Why aren't class variables
> important enough to document? I think everything exposed by a class -
> variables, methods, constants, etc. - are all equally important and should
> be documented.

The problem is finding the storage for them. Classes/functions have
internal structures that we can store doc comments in. Constants and
variables, alas, don't.

-Andrei                                       http://www.gravitonic.com/
* Proximity bug: when the program crashes in front of important visitors. *