bugGNU Octave - Bugs: bug #36743, Octave:language-extension warning...

 
 

bug #36743: Octave:language-extension warning warns about Octave's own files

Submitted by:  Jordi Gutiérrez Hermoso <jordigh>
Submitted on:  Thu 28 Jun 2012 01:59:14 PM UTC  
 
Category:  Interpreter Severity:  1 - Wish
Priority:  1 - Later Item Group:  Regression
Status:  Patch Submitted Assigned to:  Jordi Gutiérrez Hermoso <jordigh>
Originator Name:  Open/Closed:  Open
Release:  dev Operating System:  Any

Add a New Comment(Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

( Jump to the original submission)

Fri 15 Jul 2016 11:54:27 AM UTC, comment #16:

Here is a freshened version of the first part of the patch.

(file #37887)

Lachlan Andrew <lachlan>
Sat 02 Jul 2016 01:10:35 PM UTC, comment #15:

This time with the files...

(file #37667, file #37668)

Lachlan Andrew <lachlan>
Sat 02 Jul 2016 01:06:04 PM UTC, comment #14:

Here are two patches that allow pragmas to control parse-time warnings.

The first is a slight refinement of the previous patch; it just allows variable whitespace between the arguments, and makes it easier to add other pragmas later.

The second allows PKG_ADD to specify parse-time warnings for all files in the directory.

Neither of these controls run-time warnings. However, I have found them useful for reminding me when I use Octave's language extensions in code that is intended to be Matlab compatible.

I'm not sure if this partial solution is considered sufficient to make it into 4.2.0, but I'd love to see it there.

I'm retagging this as "regression", because it used to be possible to set warnings for "automatic broadcast" and it no longer is. (Unfortunately, these patches don't address that since it is a run-time issue.) That is a very easy regression to fix.

Lachlan Andrew <lachlan>
Mon 25 Apr 2016 03:41:26 AM UTC, comment #13:

Following up from jwe's comment #9, I'm not sure if the PKG_ADD file should specify run-time warnings. That would mean it would need to be 'stat'ed every time a function file is to make sure that it didn't change. Would that overhead be excessive?

It also involves much more bookkeeping than simply changing parse-time settings. The attached patch allows parse-time pragmas per file. (This is just a minimally tested proof-of-concept -- please do NOT commit it.)

I've been thinking of a big overhaul of the warning system to make warnings more flexible and, perhaps, faster.

The flexibility would come from having a hierarchy of "scopes" of warnings:
local_only (current stack frame)
file (from pragma in current file)
directory (from pragma in PKG_ADD)
local (current stack frame and all descendants)
global

At each scope, a warning's state can either be specified explicitly, or fall through to the next lower scope.

Each warning has an 8-bit state, consisting of four 2-bit fields.

1. Action: fallback, off, on, error
2. verbosity: fallback,
quiet (set lastwarn only),
verbose (print ID, to allow it to be turned off),
normal
3. backtrace: fallback,
off,
on,
unused
4. debugging: fallback,
off,
on,
unused (perhaps "on and beep")

Fallback first falls back to the status of "all" in the current scope, and if that is also fallback, it falls back to the next lower scope.

If the state is "off", the remaining fields are ignored. (This isn't quite what Matlab does; it treats "off" as "action=on, verbosity=quiet".)

Instead of using an octave_map, the octave: namespace would use a look-up table, autogenerated from warning_ids.m. From C++ code, it could be accessed through a enumeration. From .m code, it would be accessed by a binary search or hash table.

This also allows throwing errors when the state of a non-existent warning is set -- useful for example when a warning is re-named, or merged with another (like octave:automatic-broadcast was).

Having lookup more efficient would mean that we could have finer-grained IDs to give more flexibility about what is to be turned on/off.

The code to set initial states and define the enum of states could be generated from warning_ids.m, to keep them in sync. That way, when introducing a new ID or the default of an existing one, it is only necessary to change one file.

A similar framework could apply to debug_on_...

To keep the overhead of creating a stack frame low, a stack frame would start with a NULL pointer to a warning_options object, and only create the object if a "local_only" is called.

One gotcha will be ensuring that "warning on ID" updates the global scope, and also ensures that the new state isn't overridden by one of the higher scopes -- unless we want it to be. The purpose of the directory and file scopes is to override everything, so we would probably just want to make sure that local and local_only are set to fallback.

My planned work order would be
1. Replace the current warning_options (an octave_map) by a dedicated err_warn_debug object, keeping it as the same octave_map internally.

2. Implement per-directory parse-time pragmas. (That is enough for this bug.)

3. Implement "Action = fallback".

4. Replace octave_map by look-up table (for "Octave:" namespace) + STL map (for everything else).

5. Implement remaining state fields

6. Re-implement "local" warnings. (This may need to be done in conjunction with 4.)

7. Implement "file" and "local_only" scopes.

What do people think?

(file #36991)

Lachlan Andrew <lachlan>
Mon 21 Sep 2015 12:18:44 AM UTC, comment #12:

> Sounds to me, like the only thing you want is to enable the automatic broadcasting warning only. This is nothing to do with Matlab compatibility and a small part of the original bug report.

No, that is the side-discussion we have gone down. The problem is that, while working around the broadcast issue, I re-discovered this bug. That means that the bug is still alive, and still affecting people.

> My point was why should we treat automatic broadcasting different from everything else that we do different from Matlab.

The reason is the same as warning about "if (a = b)". It is valid code to can easily result from an error. The error is difficult to spot (in fact, much harder to spot since the syntax is identical), and causes misleading behaviour at a point distant from actual error.

Again, I'm not arguing that this should has to be enabled by default, but the ability to flag this condition was there. It costs no extra code to include it.

Incidentally, the suggestion that every time I use a binary operator I check the operands is entirely unworkable. Consider an expression like
A = b + c * d.
It would have to become

> The warnings were during 3.6 and 3.8. That's a 2 years warning.

People are developing code today. They are introducing bugs today. They need warnings today.

Just out of interest: What fraction of Octave users do you think are using Octave because of its differences from Matlab, and what fraction do you think are using it because it is the most Matlab-compatible of the free work-alikes? I'd guess a ratio something like 1:20. If you think it is vastly different, I'd love to hear your evidence (other than a survey of the developers, of course).

I'm sorry if I'm getting too fired up about this, but I want Octave to be useful as a real development environment (i.e., developing application in Octave, not developing Octave). Bugs happen. The interpreter should make it easy for the user to fix them, by giving the user as much information as possible. I'm going through real hoops to improve this in bug #45957. Why are you so unwilling to consider fixing a one-line regression?

My own source tree is fine -- I've re-enabled the broadcast warning. However, I want to make Octave a better product, and am getting frustrated in my attempts.

Lachlan Andrew <lachlan>
Sat 19 Sep 2015 01:23:51 AM UTC, comment #11:

> I wasn't aware that automatic broadcasting used a different syntax. [...]


A misunderstanding over the meaning of "syntax". Yes, it is not a separate symbol for automatic broadcasting. My point was why should we treat automatic broadcasting different from everything else that we do different from Matlab.

> You also refer to "doing input checking". As far as I can see, that is impossible. [...]


I mean to check the sizes of your vectors before doing your operations. Or to just reshape them (which is what most often happens). All of Octave plays with the same "rules" as you, so this is perfectly possible.

If your functions expect two vectors, your functions should check if they are of the right size and throw an error if they are not.

> [...] As it stands, it will fail the next time I assume that length(c)=2, at a place far from where the real error is.


You should have checked if `a` and `b` where both vectors of same shape. If you are typing at the interpreter, you will catch the error pretty soon. If this is a function part of some library, it should perform input checking.

> automatic broadcasting (...) is something that we should have the option of being warned about.


The warnings were during 3.6 and 3.8. That's a 2 years warning.

>> If the reason is that you didn't saw warnings back in 3.8 [...]
> No, I did see warnings back in 3.8, as I wanted.


I meant that you didn't saw warnings about use of broadcasting from inside core functions. And that's because the code in core and in packages disabled the warnings locally. Automatic broadcasting is Octave valid syntax so functions should not have to disable it.

-----

Sounds to me, like the only thing you want is to enable the automatic broadcasting warning only. This is nothing to do with Matlab compatibility and a small part of the original bug report. It is about a syntax that you don't like and where you trip too often.

Carnë Draug <carandraug>
Project Member
Thu 17 Sep 2015 03:21:16 AM UTC, comment #10:

Carnë,

> What's the reason to place the extra syntax "automatic-broadcasting" in a separate "bucket" than the others extra syntax?

I wasn't aware that automatic broadcasting used a different syntax. I thought that "automatic" meant that it uses the same syntax as non-broadcast operations. If the syntax is different, please tell me the difference so I can avoid it. You also refer to "doing input checking". As far as I can see, that is impossible. The syntax "a+b" is used both for automatic broadcasting and simple addition.

> Why would you want an error?
The reason I want an error, is so that I can fix the bug. If ever I write

I always intend to get either [3 6] or [3;6]. The fact that I don't means I made a mistake, and the interpreter should help me to find where that mistake is. As it stands, it will fail the next time I assume that length(c)=2, at a place far from where the real error is.

I would love to have a syntax for broadcasting that is less cumbersome than bsxfun (like prefixing the operator by a ), but _automatic broadcasting (i.e., overloading existing syntax, which is what I believe is the case) is something that we should have the option of being warned about.

> If the reason is that you didn't saw warnings back in 3.8 [...]

No, I did see warnings back in 3.8, as I wanted.

In my own source tree, I have restored the "Octave:broadcast" warning so that I can turn it on. Like all warnings, it is flagging an operation that is likely to be unintentional. Users should be given as much flexibility as possible in specifying what coding style is intentional and what isn't. By all means have the warning off by default, but:
(a) the warning has a clear use. See for example bug #45761.
(b) it used to be available (not tied to a broken warning)
(c) the documentation at https://www.gnu.org/software/octave/doc/interpreter/Broadcasting.html still states that it is available

Why change what wasn't broken?

Lachlan Andrew <lachlan>
Wed 16 Sep 2015 01:03:19 PM UTC, comment #9:

The language extension warning could be useful, even if it is not complete and doesn't catch every possible thing that is different from Matlab.

It is pretty useless, however, if it flags "system" code in Octave as well.

Marking functions as "system" or somehow otherwise exempt from this class of warnings makes sense to me, it's just a matter of how to do it.

We could introduce some kind of "pragma" comment that would apply to a single function:

so that warnings could be disabled for the given function when it is parsed and evaluated.

To make this a global thing for a directory (or possibly a directory tree) we could place pragma directives in a special file that Octave reads when it parses function files (or adds the directory to the load path). If we do this when the directory is added to the load path, then perhaps we could use the existing PKG_ADD file for this purpose.

Using a per-directory file might be better than maintaining a list separately (by naming them in some other startup file, for example).

John W. Eaton <jwe>
Project Administrator
Wed 16 Sep 2015 12:43:27 PM UTC, comment #8:

Lachlan Andrew said on comment #7:

> [...] That needs to be a separate warning (as it was in 3.8, IIRC).


Why is that? What's the reason to place the extra syntax "automatic-broadcasting" in a separate "bucket" than the others extra syntax? If the reason is that you didn't saw warnings back in 3.8, that's because all the code in packages and core that made use of it, turned off the warnings locally. This is not acceptable. A developer should not have to disable warnings to make use of standard language syntax.

> [...] Most of my use of Octave is for lists rather than true vectors. If I add a 1xn to an nx1, I want either to get an n-dimensional vector, or an error.


Why would you want an error? Does your code on Matlab relies on getting an error to choose an alternative codepath? I can't imagine another reason where an error would be desirable and even then, it could be argued that you should be doing input check rather than relying on an error.

> [...] Then I get inundated with errors. Even if I didn't, it would preclude the many useful Octave extensions, like +=, ++, (A+B)(10), which are syntactically different and so presumably intentional.


This confuses me. I thought the issue was all the matlab incompatible syntax. This sounds like you just don't want automatic broadcasting which is a whole different issue. And it's akin to not liking that "*" or "^" does matrix operations because you don't do matrix operations, and so wanting a switch to disable them.

> @Carnë, I can see a case for getting rid of the option. However, the overhead of checking syntax is only incurred when the file is parsed -- when it is first read in -- not every time it is executed. (Try turning the warning on, and watch it appear only when a new code path is executed.)


The warning is only issued but that's because it checks everytime, if the user has been warned before.

Carnë Draug <carandraug>
Project Member
Wed 16 Sep 2015 01:00:31 AM UTC, comment #7:

@Mike, the reason I said "syntactically" is that I was aware of the distinction that Carnë was making, and wanting to separate behavioural issues from syntactic ones. In particular, I was thinking of automatic broadcasting, which really bugs me. That needs to be a separate warning (as it was in 3.8, IIRC).

Most of my use of Octave is for lists rather than true vectors. If I add a 1xn to an nx1, I want either to get an n-dimensional vector, or an error. I don't want to get a matrix, which causes an error in an unrelated line of code. What is my solution? Turn on "warning("Octave:language-extension")". Then I get inundated with errors. Even if I didn't, it would preclude the many useful Octave extensions, like +=, ++, (A+B)(10), which are syntactically different and so presumably intentional.

It would be very easy to have a "systempath" variable, and exclude static syntax checking for all files read from that path. This only needs to affect the parser.

@Carnë, I can see a case for getting rid of the option. However, the overhead of checking syntax is only incurred when the file is parsed -- when it is first read in -- not every time it is executed. (Try turning the warning on, and watch it appear only when a new code path is executed.)

Lachlan Andrew <lachlan>
Tue 15 Sep 2015 03:58:06 PM UTC, comment #6:

The problem of discerning between "system" m-files and "user" m-files is a big one. We would not want this warning to appear for m-file functions that are part of Octave, or part of any Forge packages. Package m-files are outside of the Octave directory structure, they are not technically part of Octave. But if we have this "user" mode of the warning, we don't want warnings to be emitted for those files that are intentionally using Octave extensions.

@lachlan: when you say "syntactically?" are you saying you want this warning to only apply to syntax differences, not functional differences?

carandraug's point is that the function differences problem is much greater, even if the user writes code that uses the same punctuation and keywords as Matlab, there is still not much guarantee that it will actually work in Matlab, either because they are using Octave-only functions, or using functions common to both, but in an Octave-only way.

Updated summary to new warning name.

Mike Miller <mtmiller>
Project Administrator
Tue 15 Sep 2015 02:23:35 PM UTC, comment #5:

Maybe this warning ID ought to just be removed completely.

Here's my argument. This warning ID is only useful to write Matlab code. For that purpose, it needs to identify Octave only syntax (auto-increment, double-quotes, endifs, etc) and use of functions with options not present in Matlab.

We are only covering the first case which is very very easy for the user to identify visually, and an even easier job for any text editor (exception for automatic broadcasting).

The second case is much more difficult for the user and therefore the most useful. This would require us to go through all the octave code, and throw warnings when a specific codepath is followed (for example, calling linspace() with a vector instead of scalar). For it to work properly, according to this bug report, it would also require that a function knows if it's being called by a "user" or a "core/library" function. We shouldn't be polluting all the octave and packages code with such warnings, that would be a lot of work to start, and a lot of maintenance.

If we are not going to cover the second case, this warning is pretty much useless and will only provide a false sense of security to the user. We might as well remove and be done with this.

Also, I wonder how much of a performance issue is to check at every endif, endfunction, autoincrement, automatic broadcasting, etc, the warning status of the id.

Carnë Draug <carandraug>
Project Member
Sun 13 Sep 2015 07:36:17 AM UTC, comment #4:

I just re-read my bug report, and found that I had suggested a way to address this. Since my bug report was closed, I'm repeating the suggestion here.

I propose that we:
1. Have a new mode for warnings. As well as "off", "on" and "error", there could be "user", which turns the warning on in user code but off in library code. There could also be "user-error" to raise errors if the condition occurs in user code.
2. Overhaul the places Octave:language-extension is raised, so that it reliably raises errors for code that is (syntactically?) incompatible with Matlab.

Of course, I'm happy to be shouted down... I'd like to implement this (even though it is the lowest priority), and so want to do it in an acceptable way.

Lachlan Andrew <lachlan>
Sun 13 Sep 2015 07:27:51 AM UTC, comment #3:

This comment is so that this bug turns up on a search for "Octave:language-extension".

(My bug report about that warning was marked as a duplicate of this one, but a search for that warning didn't return this bug report, since the warning_id changed.)

Lachlan Andrew <lachlan>
Fri 29 Jun 2012 03:43:17 PM UTC, comment #2:

We currently consider "system" files to be any that are installed anywhere under the

directory. By default, these files are skipped when checking timestamps.

So it seems to me that any file that would be considered a "system" file should be excluded. So if you install packages under this directory, they would be excluded. I don't think we should give special treatment to any other files by default.

But there should be ways to mark individual directories using a command that could go in a PKG_ADD file. That would allow individual Octave Forge packages to declare that they are exempt from the Matlab incompatible warning.

Further, we might also consider some way for individual files to declare that they are exempt. Perhaps this could be done with a special comment similar to #pragma in C or C++.

For the per-directory and per-file settings, we should probably think about whether to only handle the matlab-incompatible warning, or make per-directory and per-file settings more general so they can be applied to other things as well.

Currently, we only check for matlab incompatible code in the lexer, so the check should not be too expensive. It should be as simple simple as checking a flag in the gripe_matlab_incompatible function. But doing that will require a little refactoring since we don't currently stash the "system file" designation until after the file is parsed.

Also, if we expand matlab compatibility warnings to features that can only be checked when a function is actually evaluated, then things will be more complicated. For example, suppose we want to have a function warn that it was called in a matlab-incompatible way, but not if it is called from a function that was marked as exempt from the warning. Doing that requires looking at the call stack. It's possible, but will require a little more work.

John W. Eaton <jwe>
Project Administrator
Fri 29 Jun 2012 10:20:40 AM UTC, comment #1:

I suppose Octave's "initial path" include OF packages?
i.e.,
<OCTAVE_HOME/share/octave/packages/
and
~/octave/ ?
(...which can change during the day as pkgs get loaded/unloaded)

AFAIU this warning_id is only useful for development code meant to also be run with the competition (ML / Freemat / JMathlib / (Scilab (after translation)) so your suggestion makes sense to me.

For a start, the entire <OCTAVE_HOME>/share/ tree can be excluded from this warning_id.

~/octave/ can be trickier as:
- on Linux that's where OF pkgs end up when installed as non-root (at least on my box), so it should be excluded for this warning_id, but:
- on Windows that's where I suppose people do their dev work (based on a.o., the 3.2.4 binary which created that directory) so it should be included for the warning_id.
AFAIK on Windows, m-files in OF packages are always installed in <OCTAVE_HOME/share/octave/packages/

Wouldn't checking for path in warning() induce a lot of overhead in the parser?

Philip Nienhuis <philipnienhuis>
Project Member
Thu 28 Jun 2012 01:59:14 PM UTC, original submission:

This warning id is almost useless because then almost every command warns, since Octave's m-scripts are written in Octave style.

I am investigating a way to make it warn only for non-Octave files, which I think should be files not in Octave's initial path.

Jordi Gutiérrez Hermoso <jordigh>
Project AdministratorIn charge of this item.

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #36991:  pragma_warning added by lachlan (4KiB - application/octet-stream)

 

Depends on the following items: None found

Digest:
   bug dependencies.

 

Carbon-Copy List
  • -email is unavailable- added by lachlan (Posted a comment)
  • -email is unavailable- added by mtmiller (Updated the item)
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by philipnienhuis (Posted a comment)
  • -email is unavailable- added by jordigh (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can add your encouragement to it.
    This task has 0 encouragements so far.

    Only project members can vote.

     

     

     

    Follow 13 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2016-07-15 lachlan Attached File- => Added bug_36743_pragma_warnings_part_1.v2.cset, #37887
    2016-07-02 lachlan Attached File- => Added bug_36743_pragma_warnings_part_1.cset, #37667
        Attached File- => Added bug_36743_pragma_warnings_part_2_per_dir.cset, #37668
    2016-07-02 lachlan Item GroupFeature Request => Regression
        StatusConfirmed => Patch Submitted
    2016-04-25 lachlan Attached File- => Added pragma_warning, #36991
    2015-09-15 mtmiller SummaryOctave:matlab-incompatible warning warns about Octave\'s own files => Octave:language-extension warning warns about Octave's own files
    2015-09-12 mtmiller Severity3 - Normal => 1 - Wish
        Priority5 - Normal => 1 - Later
        Item GroupIncorrect Result => Feature Request
    2015-09-12 mtmiller Dependencies- => bugs #45878 is dependent
    2014-01-19 mtmiller Dependencies- => bugs #40983 is dependent
    2012-11-21 jordigh Dependencies- => bugs #37786 is dependent

    Back to the top


    Powered by Savane 3.3