bug when adding an extension method that is an overload of a BCL class

[jonathandavid]

Consider:

extend String
	def split(chars as char*) as List<of String>
                 return ['dummy']
                        
class Foo            
	def main is shared
		print 'a:b'.split(@[c':'])       # Prints ['dummy'] 

When the main method is executed, it looks as if the compiler is generating a call to my split(char*) extension method. This is wrong, because there is also a split(char[]) method in String that matches exactly the argument I'm passing.

I think the problem only occurs when adding an extension overload to a BCL class because the problem does not show up when I try to reproduce it using a class written in Cobra:

class Foo
	def bar(x as int[])
		pass
extend Foo
	def bar(x as int*)
		.bar(List<of int>(x).toArray)   
class Baz
	def main is shared
		Foo().bar(@[1, 2]) # correctly calls array version

[jonathandavid]

First I realized that Cobra was interpreting (Compiler.readAssembly) the String.Split(char[]) method as if it took a vari parameter. I thought that had to be a bug in Cobra, but couldn't find it anywhere. So finally I checked the MSDN docs to find out that String.Split  actually takes a ParamArray char []. That is, Cobra was right in interpreting it as vari. So there was no bug after all. The only bug is in MSDN itself, beacuse  in the page that I had initially consulted, the param of Split was listed as a plain char[]. Thus my confusion. Thanks Microsoft for wasting a few hours of my life. The good part is that know I now a lot more about the internals of the Cobra compiler...

Having said that, I think there might be a related bug in Cobra, because the C# signature f(params T[] t) is translated by Cobra into f(t as vari T[]), which is I think not the right translation. The right translation would be simply f(t as vari T). I don't know if the difference is significant, maybe Cobra treats both signatures in the same way.

Also, I've discovered that if there are two overloads, f(t as vari T) and f(t as T*), and we try to pass a T[], Cobra prefers the stream overload. I don't know if that's the right thing to do, to me both options would be acceptable. I've checked how C# deals with this, and it behaves exactly the opposite way, i.e., it chooses the params T[] version instead of the IEnumerable<T> version. So maybe Cobra's overload resolution algorithm (Expr._computeBestOverload) should be adjusted so that it behaves like C# on this respect.

[jonathandavid]

I'm positive now that what I referred to in the third paragraph of my previous post is indeed a bug in the overload resolution algorithm. When faced with a T[], the compiler should *always* prefer the overload that takes a vari T[]. Otherwise, I cannot implement a method that takes a T* in terms of the version that takes a T[]. The problem shows up quite naturally when I try to add a char* overload to String.Split.

	def split(chars as char*) as List<of String>
              return List<of String>(.split(chars.toArray))

Here, with the current overload resolution algorithm, this is an infinite recursion, when it should simply invoke the BCL's String.Split(vari char[]) method.

I will see I can change the overload resolution algorithm a little bit so that it chooses the vari version as expected. Normally I would not fiddle with such a key part of the compiler, but I'm confident my back is covered by all the unit tests. If I break something, the unit tests will tell me.

[jonathandavid]

I've tweaked the overload resolution algorithm so that now the "vari T" overload is used for a T[] argument, instead of the T* overload. I'll post the patch here as soon as I figure out how to create one. I also want to write one or two tests that ensure that the new behavior is not altered in future modifications of the compiler.

By the way Chuck, I'm a little bit confused by the way VariType is structured. As you know, it's a composition that looks like this:

VariType[T] = WrappedType[NilableType[ArrayType?[T]]].

As you can imagine, the code to extract the T from a VariType gets quite convoluted. Why not have VariType as VariType[T] = WrappedType[T]? If case some of the functionality of ArrayType? is needed, why not have VariType inherit from ArrayType?? And most important, why put that Nilable layer in the middle?

Should I create a new Ticket that deals to the simplification of VariType? I can take care myself if you want...

[jonathandavid]

Who put all those quotation marks after VariType, WrappedType, etc?

[jonathandavid]

Of course I meant "question" marks

[jonathandavid]

I realize now that a "vari T" parameters can take 0 arguments, which may explain the need for a Nilable[Array[T]] inside the structure of VariType. However, it should be possible to use an Array of 0 elements instead of a nil Array, shouldn't it?

[jonathandavid]

Patch to fix the overload resolution algorithm so that now it correctly prefers a vari T overload over T*

[jonathandavid]

I've added the patch that solves this bug. It also contains a test case.

Chuck: I've read this in the How-To-Submit-A-Patch section:

Release Notes ¶30

Update IntermediateReleaseNotes?.text in the Developer directory next door to the Source directory. Include your name or forum/Trac handle for credit. Reference tickets using Trac syntax with "ticket", colon (:) and number. (The HTML formatter for the release notes supports that.)

I'm a little confused with these instructions, can you elaborate on what exactly should I add to that release notes text file?

[Chuck]

When I tried the patch and ran testify, I got an assert failure on the new test case. Are you sure this works for you?

>testify ..\Tests\120-classes
...
Internal exception: Cobra.Lang_ert_....AssertException: 
sourceSite = .. Types.cobra:1709 in VariType.trueInner.get ...
info       = nil
this       = VariType-sh(84355, ...
    (inner1.getType == NilableType) = false
        inner1.getType = IntType (RuntimeType)
            inner1 = IntType-sh(78214, ...
        NilableType = NilableType (RuntimeType)

   at VariType.get_TrueInner() in Types.cobra:line 1709
   at ArrayType.IsAssignableTo(IType type) in Types.cobra:line 1656

Regarding IntermediateReleaseNotes.text, just give a simple statement like:

* blah blah. ticket:146
# in the case of a bug:
* Fixed: <bug statement> ticket:<n>

Scroll through that file and you will see examples.

Regarding "convolutions", I did not make vari inherit from array because it wasn't clear to me if this would hold on other backends like JVM and ObjC. So I chose the more strict approach. We can always relax it later without breaking anyone's code.

NilableType should be there if and only if the type can be nil. That's separate from whether something is variable.

I'm not sure what you mean by WrappedType being in there. It's an abstract class, not instantiated at run-time.

Re: simplication of VariType I agree with you. Yes to ticket and patch. Thanks!

Also, when you work on a ticket, feel free to choose "Accept" on the ticket so it becomes assigned to you. Then you can assign to me when submitting a patch or needing an answer to a question.

[jonathandavid]

You're right, I had not run testify properly. I will see what's going on and post a new patch.

[jonathandavid]

I have run testify now. Three questions:

1) How do you get such a detailed error message? When one of the tests fail in my machine, all I get is a message box without much substance and a message that says "failure" on the console.

2) You're right that my test fails, but so do many others, which I think are not my fault (for instance, 110/120-primitive-type-members). Is it because I'm using the latest version, which is still under development? If so, how do I distinguish tests I've broken from tests that were already broken?

3) With respect to test 110/120-primitive-type-members, why does it fail with cobra -testify, but not with cobra -test? What is the difference? I must be doing something terribly stupid, please enlighten me.

[jonathandavid]

I've found the problem (don't you love it when people answer their own questions?).

I was having random test failures due to culture-related issues. In particular, my .NET installation follows the convention of writing fractional numbers with a comma instead of a dot separating the whole and the fractional parts. This is not something that is directly set in .NET, but rather at the OS level, so I'm kind of reluctant to change it just for the sake of making Cobra tests pass.

With regard to the -test/-testify conundrum, I see that -testify runs the whole cobra file (so it's equivalent to not specifying it, in the case where only one file is specified) where as -test runs the tests of the classes and methods inside the file.

[jonathandavid]

I've changed my Windows regional info from Spain to USA, and all tests pass beautifully (except the one I had broken myself, of course). My keyboard works the same which was my main concern, so I'll leave it like this for the time being. Of course, a better solution should be found as other users might not be so keen on tweaking their Windows settings.

[jonathandavid]

Regarding VariType: apparently there are two varieties of VariType. When it comes from a DLL, it looks like I described above (wrappedtype = nilable[array[T]]. This is what I wanted to simplify.

However, we must be careful because if the nilable comes from a pure Cobra code, then it has the obvious structure (wrappedtype = T). So I think my goal should be to modify the assembly loading code, so that when it loads a vari parameter, it uses the simple structure without the nilable[array[T]] part. I will write another ticket for that, and postpone my work on the current one until the other is finished.

Chuck: I was too quick to say that all tests work now. In fact the following tests fail, even after changing my locale to US:

(630) 410-use-lowernames-dll.cobra
(435) 500-cannot-infer.cobra
(127) 504-sharp-error.cobra
(635) 520-debug.cobra

This has nothing to do with any modifications I might have made: these tests fail even with a freshly downloaded version of Cobra.

Most of the failures have to do with language related issues, mostly in the code generated by C# (which is in Spanish in my installation, even though I don't remember having selected that option anywhere during the installation of the framework). Except one of them, 520-use-lowernames-dll.cobra, which seems to fail because the referenced dll does not exist.

[Chuck]

"cobra -testify" outputs a file called r-testify with mucho details. When a test fail, it is run a second time with more details. The "r" in "r-testify" is for "results" and it's just a convention I use for these types of output files. Then I can safely delete "r-*" at any time.

The last time I ran testify on Windows (yesterday) all 777 tests passed.

We fixed the cultural issues at one point (the other user was from Sweden), but they must have crept back in. If you search the code for "culture" you will find several references to "Utils.cultureInfoForNumbers". Are you able to fix these problems? Maybe CobraTokenizer is parsing without using this.

What is your op sys? Are you on .NET or Mono, and what version?

[jonathandavid]

I'm on XP with .NET 2.0. I'll investigate these failing tests myself, and hopefully write a patch if I get to make them work.

[Chuck]

It would be very much appreciated! I'm unable to investigate these problems myself.

[jonathandavid]

Chuck, I don't know if the following is a bug or a feature:

class Test

	def takesVari(x as vari String) is shared
		for y in x
			print y

	def takesArray(x as String[]) is shared
		for y in x
			print y
		
	def main is shared
		
		.takesVari(@['hola', nil])  # compiles fine, no runtime assertion 

		.takesArray(@['hola', nil])  # does not even compile

I would've expected the two versions (vari and []) to behave identically. That is, I the call to "takesVari" to cause a compilation error.

Cobra accepts the syntax fun(x as vari String?), but the resulting code behaves as if the "?" wasn't there at all.

If I'm not mistaken, this would require a separate ticket. And to think that this all started because I wanted to a simple overload of String.split... :)

[Chuck]

Yeah sounds like a separate ticket.

[jonathandavid]

About the failing tests:

So far I've been able to fix the following ones:

(29) 062-string-substitution.cobra
(85) 120-primitive-type-members.cobra
(101) 310-primitive-members.cobra
(133) 510-number-parse.cobra
(267) 100-number-float32.cobra
(269) 100-number-float64.cobra
(273) 106-number-decimal.cobra

I've fixed all of them by adding CultureInfo?.invariantCulture when calling toString or parse. Otherwise, my locale turned "3.2" into "3,2", which wasn't expected by the tests.

I encountered my first problem with test "(130) 504-sharp-error.cobra". This test assumes that the C# will produce error messages in English, which is not the case in my installation.

A similar thing happens with "(445) 500-cannot-infer.cobra", and with "520-debug.cobra". I think we can relax these tests a little bit so that they don't look for specific messages, but rather for things that won't change on a non-English locale. For example, instead of looking for a warning that contains "related to", we can look for a warning that contains simply the line that causes the warning "(3)". If you want I can perform these modifications, and once all tests pass in my installation, create a patch and submit it here.

A different kind of problems occurs with "(639) 410-use-lowernames-dll.cobra". This took me a while to figure out. Apparently, the test is referencing a lowernames.dll file that should be in the current path. Instead, there is a lowernames.cs file. I got the test to pass by manually compiling this .cs file, which generates the necessary dll, but this is not done automatically when I install from source and run -testify. I don't know how to solve this problem, can you help me?

Also, there seems to be a minor issue with the testify script itself. When a test fails, the test count is incremented twice, and the test name is written twice. For example:

       (636) 400-foolib.cobra
        (637) 402-fooprog.cobra
        (638) 404-fooprog.cobra
        (639) 410-use-lowernames-dll.cobra
        FAILURE ----------------------------------------------------------------------
        (640) 410-use-lowernames-dll.cobra
        (641) 500-sharp-args.cobra
        (642) 510-lib-option-prep.cobra
        (643) 512-lib-option.cobra
        (644) 520-debug.cobra

[Chuck]

Thanks for the report. We have to move this to ticket:153 and keep this ticket dedicated to its original purpose about extension methods.

I have responded at ticket:153