bugGNU Octave - Bugs: bug #46849, Shadowing warning on every command...

 
 

bug #46849: Shadowing warning on every command for function in +package directory

Submitter:  Richard <crobar>
Submitted:  Fri 08 Jan 2016 03:06:35 PM UTC
   
 
Category:  Interpreter Severity:  3 - Normal
Priority:  7 - High Item Group:  Unexpected Error or Warning
Status:  Confirmed Assigned to:  None
Originator Name:  Richard Crozier Open/Closed:  * Open
Release:  * dev Operating System:  * Any
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Sun 20 Dec 2020 12:24:35 PM UTC, comment #10: 

I didn't see the caching (but it is very well possible that I just didn't understand it).
AFAICS, `load_path::update` effectively purges the load path and re-builds it from scratch. So IIUC, atm there is no way of detecting whether a particular function is new on the load path or whether it was already shadowing before.
I don't know how the repeated shadowing warnings can be avoided.
There is probably a better way of doing this. But that is probably outside the scope of this report.

I tried some primitive changes to add the function name including the namespace to the map in load-path.cc. That helped to suppress the warning. But also made every function in a +namespace completely unusable.

Since jwe is already planning on overhauling this part of the code, I'll leave this to him.

Markus Mützel <mmuetzel>
Group administrator
Sat 19 Dec 2020 04:48:51 PM UTC, comment #9: 

Overhauling the load path code has been on my wish list for a while.  I can't promise but I hope to find some time to work on it for Octave 7.

John W. Eaton <jwe>
Group administrator
Fri 18 Dec 2020 06:02:11 PM UTC, comment #8: 

Octave is expected, and thus designed, to pick up changes in scripts and the addition of new functions automatically.  Reversing that assumption would be a major design choice that would need to be discussed among the maintainers over on Discourse.

I also agree though that I've never been particularly happy with the performance consequences of this choice.  Some of it is ameliorated in that the load_path system caches the modification times on the directories and doesn't actually read each directory every time.  Instead, it just does a check of the modification time and compares it to the last time Octave parsed the directory.  Most (>99%) of the time nothing has changed and so the test is quick.  On most UNIX-based operating systems inodes are cached if there is sufficient memory so checking the timestamp doesn't even mean a read of a slow device like a disk drive.  Instead, it is just a memory read.

Rik <rik5>
Group administrator
Fri 18 Dec 2020 05:05:10 PM UTC, comment #7: 

It is a nice feature of Octave (and matlab) that code changes are picked up automatically and immediately in comparison to, e.g. python where the mechanism is much more clunky. Plus, when working on the command line, speed usually isn't that much of an issue between commnads.

It would be really nice to have this fixed, it's quite an embarassing and visible problem for users coming from Matlab. It induces nervousness.

I agree my workaround below is almost certainly not the correct solution.

Richard <crobar>
Fri 18 Dec 2020 04:32:54 PM UTC, comment #6: 

The change in comment #2 is probably not exactly what we want.
We already have the built-in function "meta.class" (and other similar ones). A file in "+meta/class.m" would shadow that function.
We'd probably need to extract the (nested) namespace from the path and check with the correctly namespaced function.

I'm not sure how namespaced functions are handled in the maps in `load_path`. But we'd probably need to prepend the namespace to the function name before this line:
https://hg.savannah.gnu.org/hgweb/octave/file/3332f1964e59/libinterp/corefcn/load-path.cc#l1881


Looking at the code path that leads to the warning being repeated on every input:
Do we really need to update the load path after each and every command?
https://hg.savannah.gnu.org/hgweb/octave/file/3332f1964e59/libinterp/corefcn/input.cc#l841
in octave::base_reader::octave_gets

    // There is no need to update the load_path cache if there is no
    // user input.
    if (retval != "\n"
        && retval.find_first_not_of (" \t\n\r") != std::string::npos)
      {
        load_path& lp = m_interpreter.get_load_path ();

        lp.update ();


That looks like a lot of work and disc IO without any changes effectively most of the time...

Markus Mützel <mmuetzel>
Group administrator
Fri 18 Dec 2020 01:53:59 PM UTC, comment #5: 

Since it is not possible to vote for the bug, I would give some feedback as comment.

I also run into the bug while trying to convert a larger Matlab project to Octave. While the fix is rather simple, just turning off the warning, we would still have to distribute this knowledge to the potential user base. Since no-one reads documentation, I can already foresee bug reports from our user base.

Ulf Lorenz <ulflorenz>
Wed 02 Dec 2020 11:43:34 PM UTC, comment #4: 

I have the same issue still.

Additionally, contrary to some other warnings in octave which appear just once per session, this warning repeats all the time, hence easily filling the whole command window. For instance, if the package is in the search path, the warning is produced every time the working directory is changed with the "cd" command (even if the package is not in the current path).

What about incorporating the solution from Richard below? comment #2:



JD <jdbancal>
Wed 15 Aug 2018 07:34:20 PM UTC, comment #3: 

Adding jwe to the CC list and increasing the priority.  The whole point of namespaces is to allow for the same names to exist without provoking a conflict.

A little extra information, the shadowing warning is only for built-in (i.e, C++ functions).  I can put an additional 'ls.m' m-file in the +testpkg directory and I don't get a shadowed warning.

Rik <rik5>
Group administrator
Mon 14 May 2018 01:41:35 PM UTC, comment #2: 

I modified load-path.cc around line 1686 to check for a '+' in the first character of the directory name when deciding whether the function is shadowed like this:


                    symbol_table& symtab
                      = __get_symbol_table__ ("load_path::package_info::add_to_fcn_map");

                    // check it is not a package directory
                    std::string last_dir_name;
                    std::size_t lastDirPos = dir_name.find_last_of("/");

                    if (lastDirPos == std::string::npos)
                      {
                        // make sure there's something to test
                        last_dir_name = "-";
                      }
                    else
                      {
                        last_dir_name = dir_name.substr(lastDirPos+1, dir_name.length()-1);
                      }

                    if (last_dir_name[0] != '+')
                      {
                        if (symtab.is_built_in_function_name (base))
                          {
                            std::string fcn_path = sys::file_ops::concat (dir_name, fname);

                            warning_with_id ("Octave:shadowed-function",
                                             "function %s shadows a built-in function",
                                             fcn_path.c_str ());
                          }
                      }


And the warnings went away, no doubt this is not the best way to fix this though.

Richard <crobar>
Mon 14 May 2018 12:31:26 PM UTC, comment #1: 

This issue still exists in the stable dev version of Octave, I've attached a session running octave to demonstrate the problem. I get over 100 lines of warnings when launching Octave and four or five lines of warnings when running every command, even just x = 1; etc., nothing to do with the 'shadowed' functions (which aren't shadowed, they are in a package namespace).




(file #44154)

Richard <crobar>
Fri 08 Jan 2016 03:06:35 PM UTC, original submission:  


I think I have noticed an issue with incorrectly warning about shadowing of function in +package directories in the dev build (I am going to use +package to refer to Matlab style package directories, as opposed to Octave packages).

If I have a function in +package I get a shadowing warning against functions of the same name in the global namespace. In recent builds this is issued on every single command. I have done the following to test this.

create a directory call +testpckg

Created a function file 'inverse.m' in this directory (there is an Octave built-in inverse function). This new function should be in the testpkg namespace and not clash with the built-in.

The directory containing +testpckg is added to the path on startup by running genpath on a higher level directory. I have checked that the directory +testpckg is not in the path, but the directory containing it is.

On startup I get the warning:

warning: function .../mfiles/octave-test/+testpckg/inverse.m shadows a built-in function

This isn't so bad, but as I mentioned, I now also get the same warning with every single command entered at the Octave prompt.

hg id reports 63374982750b tip @

Richard <crobar>

 

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

Attach Files:
   
   
Comment:
   

Attached Files

 

Carbon-Copy List
  • -email is unavailable- added by svillemot
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by mmuetzel (Posted a comment)
  • -email is unavailable- added by ulflorenz (Posted a comment)
  • -email is unavailable- added by jdbancal (Posted a comment)
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by rik5
  • -email is unavailable- added by amro_octave
  • -email is unavailable- added by crobar (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 group members can vote.

     

    Follow 15 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2021-01-15 svillemot Carbon-Copy- Added svillemot
    2020-12-20 mmuetzel Priority6 7 - High
        StatusPatch Submitted Confirmed
    2020-12-18 mmuetzel Operating SystemGNU/Linux Any
    2018-08-15 rik5 Priority5 - Normal 6
        StatusConfirmed Patch Submitted
        Carbon-Copy- Added jwe
    2018-07-08 mtmiller Dependencies- bugs #54270 is dependent
    2018-05-14 mtmiller Carbon-CopyRemoved 80942 -
    2018-05-14 mtmiller Item GroupNone Unexpected Error or Warning
        StatusNone Confirmed
    2018-05-14 crobar Attached File- Added octave_shadow_warnings.txt, #44154
    2016-12-09 amro_octave Carbon-Copy- Added amro_octave
    2016-07-03 mtmiller Dependencies- bugs #48390 is dependent
    2016-04-04 mtmiller Dependencies- bugs #47622 is dependent

    Back to the top

    Powered by Savane 3.13-cf05.
    Corresponding source code