bugGNU Octave - Bugs: bug #51377, Parse error for abstract methods


bug #51377: Parse error for abstract methods

Submitter:  Martynas <mpata>
Submitted:  Mon 03 Jul 2017 11:27:33 PM UTC
Category:  Interpreter Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Matlab Compatibility
Status:  In Progress Assigned to:  None
Originator Name:  Open/Closed:  * Open
Release:  * 4.2.1 Operating System:  * Any
* Mandatory Fields

Add a New Comment (Rich Markup)

Comment Type & Canned Response:


( Jump to the original submission )

Wed 16 Jan 2019 09:52:37 AM UTC, comment #11: 

Hey guys. Are there any news on this?

Markus Ebner <seijikun>
Mon 18 Dec 2017 06:18:58 PM UTC, comment #10: 

The old patch is no longer compatible with tip. Therefore, I created an updated version against a51a92186a1f421a35ed2bdf63a39.

It's name is patch_ver3_against_a51a92186a1.diff

(file #42674)

Piotr Held <jsoh425>
Tue 05 Sep 2017 11:19:12 PM UTC, comment #9: 

I am very sorry however in comment #6 I attached the wrong patch... I am sorry for the confusion.

I intended to attach the patch that I am attaching now.

The comments I wrote actually refer to the code in this patch.

(file #41739)

Piotr Held <jsoh425>
Wed 30 Aug 2017 05:52:47 PM UTC, comment #8: 

I started thinking if my solution does not cause a memory leak, because I raise an error inside make_meta_class. Sure enough it seems like it did cause a memory leak. However, there was another error raised in cdef_class::make_meta_class, so the memory leak existed prior to this new cset. That memory leak happened when creating 'sl_child' in the code below:

classdef (Sealed) sl_parent &lt; handle

classdef sl_child &lt; sl_parent

So I made a patch that should solve the problem of raising errors inside cdef_class::make_meta_class and I am attachin it here. This patch should work with the current tip (23973:a315ac23dc6c).

(file #41701)

Piotr Held <jsoh425>
Fri 25 Aug 2017 11:27:25 PM UTC, comment #7: 

I am sorry, a typo got into my message:

This patch does not throw errors if you attemp to create an instance of an abstract class.

Piotr Held <jsoh425>
Fri 25 Aug 2017 11:15:01 PM UTC, comment #6: 

I wrote a new patch in the file simple_patch_bug51377.diff

This is a much simpler patch that only deals with this bug. It does not solve the attribute validation problem, though I did add a FIXME about it in the classdef creation code. I would be willing to write the attribute valiadion inside ov-classdef.cc, however, that is not necessary for this problem to be solved.

This patch does throw errors if you attempt to can create an instance of an abstract class. That is an attribute validation problem and I wanted to keep this patch as simple as possible.

Apart from fixing the bug, the only additional thing I did was refactoring tree_classdef_attribute. Previously if an attribute was negated (e.g. 'methods (~Abstract)') tree_classdef_attribute was designed to hold that information as true (as in tree_classdef_attribute::negate = true). It seems more intuitive to hold the value as false, since we are actually setting the value of the attribute to false (in the refactored version: tree_classdef_attribute::bool_value = false).

I also added a bunch of tests.

(file #41663)

Piotr Held <jsoh425>
Thu 17 Aug 2017 11:46:56 PM UTC, comment #5: 

I wanted to write down somewhere our conversation from IRC and point to the problems with my cset and how to solve parsing problem. For Octave it is important to know if a method block has attribute Abstract. This is quite easy if the declaration looks like this:

classdef mycls
  methods (Abstract)

But that is not the only allowed syntax. Attributes can be called using 'Attrib_Name = value'. This 'value' can be be anything from 'true', 'false', 'private' to functions and metaclasses. Currently the interpreter does no input validation on attributes at all and assigns their value during class creation (using the information stored in a parse-tree). I have not done this yet, but I was gonna propose to evaluate the value of an Attribute at parse-time and assign it there, so that the parse tree holds that value. Otherwise, if we evaluate the attribute value at parse-time to check what it is, we will have to evaluate it again during class creation. What do you think of this proposal?

Also does anyone have an idea how to solve the recursive attribute lookup question? (Right now I just simply avoid evaluating '?' and 'metaclass' calls when parsing attributes). My idea was to look at the call stack and see if the caller is a _make_class_meta_ call, but I think that isn't all that great.

To give examples of what I'm talking about:

classdef child &lt; parent

classdef parent
  methods (Access = ?child)
    function foo (self)

If we evaluate the value of Access at parse-time we create a recursive parse lookup when invoking the constructor of either the parent or the chld (when we create the child we need to parse it->to parse the child we need to parse the parent->when parsing the parent we look at attribute Access and then we try to get it's value->to get it's value we need to parse the child-> and so on). Any suggestions?

Piotr Held <jsoh425>
Thu 17 Aug 2017 12:21:09 AM UTC, comment #4: 

I did some more testing and found out that:
1. 'private' is supported by Matlab not only private (so string types).
2. containers.Map had a syntax error which I also fixed in this cset (I say syntax error because property 'map' was not private even though it was marked as such by the person who wrote this classdef).
3. Logical attributes accept not only logical but basically numerical scalars.

(file #41551)

Piotr Held <jsoh425>
Wed 16 Aug 2017 10:44:11 PM UTC, comment #3: 

So I have a cset that solves this problem and provides attribute validation. As I explain in the comments the intent is for it to be more permissive than not. It's weakest link is assigning attributes a metaclass values. The problem arises in recursive metaclass calls. However, this functionality isn't perfect anyway in Octave.

I would like to create a number of tests for various cases present here, however I do not know where to do that. It seems that the bug test space is not a proper place for it. Where should I put these tests?

(file #41550)

Piotr Held <jsoh425>
Tue 15 Aug 2017 05:01:22 PM UTC, comment #2: 

After some discussion on IRC, we decided to do better validation of classdef attributes in the parser as a first step toward fixing this problem.

John W. Eaton <jwe>
Project Administrator
Tue 15 Aug 2017 04:14:08 PM UTC, comment #1: 

The way I solved this problem is I created a lexical_feedback flag called looking_at_abstract_attribute_block. This is true when an attr (in oct-parse.in.yy) with name &quot;Abstract&quot; appears (or an assignment &quot;Abstract = true&quot;). If the flag is true then base_parser::start_classdef_external_method creates a dummy function for the abstract method and everything parses correctly (just as it would if the abstract class was in an @-folder).

I will open a separate bug that concerns the fact that you can instantiate abstract classes (even now if placed in @-folders an abstract class can be instantiated). I will attempt to solve that problem if the solution to this problem is acceptable.

I also added tests for this particular problem.

(file #41527)

Piotr Held <jsoh425>
Mon 03 Jul 2017 11:27:33 PM UTC, original submission:  

GNU Octave shows parse error for abstract methods in abstract classes defined with "classdef".

For example, we have an abstract class:

classdef (Abstract) AbstractIncrementClass
  methods (Static, Abstract)
    [Y] = Increment(X)

And a concrete class:

classdef ConcreteIncrementClass < AbstractIncrementClass
  methods (Static)
    function [Y] = Increment(X)
      Y = X + 1;

Running command "ConcreteIncrementClass.Increment(1)" results in error message (path removed):

parse error near line 3 of file ...\AbstractIncrementClass.m

  external methods are only allowed in @-folders

>>>     [Y] = Increment(X)

In Matlab it gives the answer (2).

Without "Static" keyword the same error message is shown while running the constructor of concrete class (Matlab shows no error here as well).

Martynas <mpata>


(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:

Attached Files
file #41739:  patch_ver3.diff added by jsoh425 (13KiB - text/x-patch)
file #41701:  memory_leak_patch.diff added by jsoh425 (1KiB - text/x-patch)
file #41663:  simple_patch_bug51377.diff added by jsoh425 (7KiB - text/x-patch)
file #41551:  patch_attrVer2.diff added by jsoh425 (23KiB - text/x-patch)
file #41527:  patch_bug51377.diff added by jsoh425 (7KiB - text/x-patch)


Depends on the following items: None found

Items that depend on this one: None found


Carbon-Copy List
  • -email is unavailable- added by seijikun (Posted a comment)
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by jsoh425 (Updated the item)
  • -email is unavailable- added by mpata (Submitted the item)

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only project members can vote.





    Follow 8 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2017-12-18 jsoh425 Attached File- Added patch_ver3_against_a51a92186a1.diff, #42674
    2017-09-05 jsoh425 Attached File- Added patch_ver3.diff, #41739
    2017-08-30 jsoh425 Attached File- Added memory_leak_patch.diff, #41701
    2017-08-25 jsoh425 Attached File- Added simple_patch_bug51377.diff, #41663
    2017-08-17 jsoh425 Attached File- Added patch_attrVer2.diff, #41551
    2017-08-16 jsoh425 Attached File- Added patch_attributeValidation.diff, #41550
    2017-08-15 jwe StatusNone In Progress
    2017-08-15 jsoh425 Attached File- Added patch_bug51377.diff, #41527

    Back to the top

    Powered by Savane 3.9