Microsoft.VisualBasic.Strings.Left(String, Int32) crashes with null ref when length is negative

[ghost]

Repo: Call method with negative Length (e.g. Strings.Left(string.Empty, -1);)
Expected: System.ArgumentException (Argument 'Length' must be greater or equal to zero.)
Actual: System.ArgumentNullException (Value cannot be null.)

Looks like it can't find the resource Argument_GEZero1 for some reason. Output from a (tweaked) test I'm working on for #19615.

Microsoft.VisualBasic.Tests.StringsTests.LeftTest(str: null, length: -1, valid: False, expected: \"\") [FAIL]
   System.ArgumentNullException : Value cannot be null.
   Parameter name: format
   Stack Trace:
         at System.String.FormatHelper(IFormatProvider provider, String format, ParamsArray args)
         at System.String.Format(String format, Object[] args)
      R:\corefx\src\Common\src\System\SR.vb(64,0): at System.SR.Format(String resourceFormat, Object[] args)
      R:\corefx\src\Microsoft.VisualBasic\src\Microsoft\VisualBasic\CompilerServices\Utils.LateBinder.vb(86,0): at Microsoft.VisualBasic.CompilerServices.Utils.GetResourceString(String resourceKey, String[] args)
      R:\corefx\src\Microsoft.VisualBasic\src\Microsoft\VisualBasic\Strings.vb(38,0): at Microsoft.VisualBasic.Strings.Left(String str, Int32 length)
      R:\corefx\src\Microsoft.VisualBasic\tests\StringsTests.cs(121,0): at Microsoft.VisualBasic.Tests.StringsTests.LeftTest(String str, Int32 length, Boolean valid, String expected)

[danmoseley]

That would happen if I had accdientally removed it this morning, but I don't think I did:

dan@dan-Lenovo-Ideapad-S12:~/git/corefx$ git show 6b4423 | grep "^-.*GEZ"
dan@dan-Lenovo-Ideapad-S12:~/git/corefx$ 

Do you see it in src/Microsoft.VisualBasic/src/Resources/Strings.resx?

[danmoseley]

It's odd it's using R:\corefx\src\Common\src\System\SR.vb. My copy is in src/Microsoft.VisualBasic/src/Resources/SR.vb

[danmoseley]

Does it repro if yo usync before (or temporily revert) 6b4423? If so I'l take a look.

[ghost]

Yep. My topic branch is still on 31cb598 which is a few hours older so it's safe to say that 6b4423f didn't cause this. Also, I can confirm that this is a more general issue as Conversions.ChangeType(Object, Type) is affected as well.

[bbowyersmyth]

Possibly related dotnet/buildtools#1256

[ghost]

@danmosemsft Any update on the resource issue?

[danmoseley]

I didn't have a chance to debug but I guess to reach this code you had to add Left to the public API? Not sure why the implementation is there but before adding it we should consider if we want to. (Not sure why we wouldn't but apparently someone made that decision)

ref\Microsoft.VisualBasic.cs

    public sealed partial class Strings
    {
        internal Strings() { }
        public static int AscW(char String) { throw null; }
        public static int AscW(string String) { throw null; }
        public static char ChrW(int CharCode) { throw null; }
    }

[danmoseley]

I notice in https://github.com/dotnet/corefx/issues/14300#issuecomment-284763018 @AnthonyDGreen says we don't want to port these. If you'd like to propose we should, do add your thoughts in that issue.

Meantime let me know if you hit this when using an current public API.

[ghost]

@danmosemsft Not really. As a (former) SDET I'm quite happy to resort to reflection (and I know that changing the public API is subject to an approval process). But given your last comment and @AnthonyDGreen's statement I'll remove Strings.Left(String, Int32). Also, I already have hit this with another, (truly) public method so I still believe that this is a general issue with the resources (see above: https://github.com/dotnet/corefx/issues/16289#issuecomment-280878364).

EDIT: Spoke too soon. The method is currently used by Operators.GetNoValidOperatorException(UserDefinedOperator, Object, Object). Now, I don't think that's a reason for not removing Left but just to be on the safe side I'd rather postpone that until GetNoValidOperatorException is covered by unit tests.

[danmoseley]

@dennisdietrich yes we definitely should debug this, it might be a few days (again) unless you get to it.

[dhoehna]

Can I work on this issue and fix Microsoft.VisualBasic.Strings.Left(String, Int32)?

[dhoehna]

@danmosemsft

[danmoseley]

@dhoehna yes please, I haven't had time and may not for a while.

As mentioned above, any new API should go through a process and @AnthonyDGreen but certaily bug fixes would be great. I'll assign to you. thanks!

[dhoehna]

@dennisdietrich How were you able to call String.Left from StringsTest? I can't call String.Left because, according to the metadata, Left does not exist. Which is odd.

[danmoseley]

@dhoehna for some reason (accident?) Left is in the implementation. I assumed @dennisdietrich added it locally to his ref file as well Microsoft.VisualBasic\ref\Microsoft.VisualBasic.cs so it could be referenced.

As I undersatnd it from this comment -- Anthony is the owner of VB -- functions like Left should get compiled-in and aren't needed in .NET Core. The only code we have in Microsoft.VisualBasic is what's needed for some dynamic scenarios.

Nevertheless anything that's actually reachable should not fail in this way and if I recall correctly, I found that some others (that are exposed) hit this. That's what I hope you can look into - starting by writing tests for stuff exposed in the ref file.

[dhoehna]

@danmosemsft Alright. So, I'll start out by expanding the stringtests class and we can go from there. Sound good? Or am I missing the point?

[danmoseley]

Sure, you can add coverage for everything in the ref, which for this class is not much

    public sealed partial class Strings
    {
        internal Strings() { }
        public static int AscW(char String) { throw null; }
        public static int AscW(string String) { throw null; }
        public static char ChrW(int CharCode) { throw null; }
    }

As you can see it's the only thing covered in the whole VB assembly, in fact we have https://github.com/dotnet/corefx/issues/14344 to fix the coverage

[danmoseley]

@dhoehna did you get a chance to take a look?

[dhoehna]

@danmosemsft I'll be looking at all the feedback today.

[dhoehna]

oh wait, wrong issue

[dhoehna]

@danmosemsft Yeah. I thought this issue was fixed and merged. Did it not get merged?

[danmoseley]

Ah yes dotnet/corefx#18182. Thanks! (If you include a link in your PR, it shows in the isseu)

I guess I was wondering whether anyone looked at the resource loading probelm or whether it's real. @ViktorHofer could you have a quick look?

[ViktorHofer]

The test method doesn't even exist anymore: findstr /si /C:"LeftTest" * --> nothing
The resource exists: findstr /si /C:"Argument_GEZero1" * --> Microsoft.VisualBasic\src\Resources\Strings.resx: <data name="Argument_GEZero1" xml:space="preserve">

I think we are safe to close. Feel free to reopen if I missed something.