R devel - Jan 2018 - Better error message in loadNamespace

Hi

I?ve just spend a bit of time debugging an error arising in `loadNamespace`. The
bottom line is that the `vI` object is assigned within an `if` block but
expected to exist for all of the remaining code. In some cases where the package
library has been corrupted or when it resides on a network drive with bad
connection this can lead to error messages complaining about `vI` object not
existing. Debugging through the error is difficult, both because `loadNamespace`
is called recursively through the dependency graph and the error can arise at
any depth. And because the recursive calls are wrapped in `try` so the code
breaks some distance from the point where the error occurred.

I will suggest mitigating this by adding an `else` clause to the `if` block
where `vI` gets assigned that warns about potential corruption of the library
and names the package that caused the error.

I can open a bug report if you wish, but I would require a bugzilla account for
that. Otherwise you?re also welcome to take it from here.

With best wishes
Thomas Lin Pedersen

>>>>> Thomas Lin Pedersen <thomasp85 at gmail.com>
>>>>>     on Mon, 22 Jan 2018 14:32:27 +0100 writes:
    > Hi I?ve just spend a bit of time debugging an error
    > arising in `loadNamespace`. The bottom line is that the
    > `vI` object is assigned within an `if` block but expected
    > to exist for all of the remaining code. In some cases
    > where the package library has been corrupted or when it
    > resides on a network drive with bad connection this can
    > lead to error messages complaining about `vI` object not
    > existing. Debugging through the error is difficult, both
    > because `loadNamespace` is called recursively through the
    > dependency graph and the error can arise at any depth. And
    > because the recursive calls are wrapped in `try` so the
    > code breaks some distance from the point where the error
    > occurred.

    > I will suggest mitigating this by adding an `else` clause
    > to the `if` block where `vI` gets assigned that warns
    > about potential corruption of the library and names the
    > package that caused the error.

Not sure this is desirable... in general even though it may well
be desirable in your use case...

You will be aware that this an important function that maybe
called many times, e.g., notably even at R startup time and so
must be very robust [hence the many try* settings] and must use
messages/warnings that are suppressable etc etc.

On reading the source, I tend to agree with you that it looks
odd there is no  else  clause to that if(), but then there may
be subtle good reasons for that we don't see now.

    > I can open a bug report if you wish, but I would require a
    > bugzilla account for that. Otherwise you?re also welcome
    > to take it from here.

I'll do that for you in any case.

Martin Maechler
ETH Zurich


    > With best wishes Thomas Lin Pedersen

> On 22 Jan 2018, at 16.21, Martin Maechler <maechler at
stat.math.ethz.ch> wrote:
> 
>>>>>> Thomas Lin Pedersen <thomasp85 at gmail.com
<mailto:thomasp85 at gmail.com>>
>>>>>>    on Mon, 22 Jan 2018 14:32:27 +0100 writes:
> 
>> Hi I?ve just spend a bit of time debugging an error
>> arising in `loadNamespace`. The bottom line is that the
>> `vI` object is assigned within an `if` block but expected
>> to exist for all of the remaining code. In some cases
>> where the package library has been corrupted or when it
>> resides on a network drive with bad connection this can
>> lead to error messages complaining about `vI` object not
>> existing. Debugging through the error is difficult, both
>> because `loadNamespace` is called recursively through the
>> dependency graph and the error can arise at any depth. And
>> because the recursive calls are wrapped in `try` so the
>> code breaks some distance from the point where the error
>> occurred.
> 
>> I will suggest mitigating this by adding an `else` clause
>> to the `if` block where `vI` gets assigned that warns
>> about potential corruption of the library and names the
>> package that caused the error.
> 
> Not sure this is desirable... in general even though it may well
> be desirable in your use case...
> 
> You will be aware that this an important function that maybe
> called many times, e.g., notably even at R startup time and so
> must be very robust [hence the many try* settings] and must use
> messages/warnings that are suppressable etc etc.
I absolutely agree that even small changes in execution speed 
would be bad in such a low-level function. Still, the else clause 
would only get called in the event an error should be thrown so I
can?t envision any performance regression.
> 
> On reading the source, I tend to agree with you that it looks
> odd there is no  else  clause to that if(), but then there may
> be subtle good reasons for that we don't see now.
If there are reasons for the current construct, then that should of
course be taken into account. As far as I can parse every code
branch that follows the if statement ends up referencing vI, but
lazy evaluation might result in those expression to never be 
evaluated so it might be valid calls in some circumstances..?

Anyway, I can accept the argument that changing it might break
things in unexpected ways :-)
> 
>> I can open a bug report if you wish, but I would require a
>> bugzilla account for that. Otherwise you?re also welcome
>> to take it from here.
> 
> I'll do that for you in any case.
thanks

best
Thomas
> 
> Martin Maechler
> ETH Zurich
> 
> 
>> With best wishes Thomas Lin Pedersen

	[[alternative HTML version deleted]]