New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Regression in Array interface conversions #7093

Closed
slide opened this Issue Feb 17, 2018 · 7 comments

Comments

Projects
None yet
4 participants
@slide
Contributor

slide commented Feb 17, 2018

Steps to Reproduce

  1. Unzip the attachment
  2. Run ```mono ipy.exe doit.py``

ironpython_parameter_issue.zip

Current Behavior

Current behavior is that the method is called successfully, you see the message "You are here" printed on the console.

Expected Behavior

This should not work as an int[] is not an IEnumerator<int>. This started failing in the IronPython unit tests on Mono 5.8. It throws an exception as expected on previous versions. On MS.NET I get the following when running the reproduction code as expected

Traceback (most recent call last):
  File ".\doit.py", line 7, in <module>
TypeError: expected IEnumerator[int], got Array[int]

On which platforms did you notice this

[ ] macOS
[x] Linux
[ ] Windows

Version Used:

Mono JIT compiler version 5.8.0.108 (tarball Fri Jan 19 18:15:21 UTC 2018)
Copyright (C) 2002-2014 Novell, Inc, Xamarin Inc and Contributors. www.mono-project.com
TLS: __thread
SIGSEGV: altstack
Notifications: epoll
Architecture: amd64
Disabled: none
Misc: softdebug
LLVM: supported, not enabled.
GC: sgen (concurrent by default)

@marek-safar marek-safar added this to the 2017-12 milestone Feb 19, 2018

@marek-safar

This comment has been minimized.

Member

marek-safar commented Feb 19, 2018

@lambdageek could you please investigate

@marek-safar marek-safar changed the title from Regression in generic method parameter handling to Regression in Array interface conversions Feb 19, 2018

@lambdageek

This comment has been minimized.

Member

lambdageek commented Feb 22, 2018

It's mono_class_is_assignable_from again, like #7095. Standalone repro:

using System;

public class Repro {
  public static void Main () {
        var a = new int [] {1,2,3};
        var t = typeof (System.Collections.Generic.IEnumerator<int>);
        Console.WriteLine (t.IsAssignableFrom (a.GetType()));
  }
}

Expected output: False. Actual output: True.

@lambdageek

This comment has been minimized.

Member

lambdageek commented Feb 22, 2018

/cc @kumpera probably time to make IEnumerator`1 not an array_special_interface?

@lambdageek

This comment has been minimized.

Member

lambdageek commented Mar 1, 2018

@kumpera I know we talked about it a few times, but I think I don't understand the full story of why IEnumerator`1 must be an array special interface. It's something like "we return the same instance from (arr as IEnumerable<byte>).GetEnumerator() and (arr as IEnumerable<sbyte>).GetEnumerator() and therefore we need IEnumerator<byte> and IEnumerator<sbyte> to have the special hacked is_assignable_from semantics. Is that right so far?

But we dont' need T[] and IEnumerator<T> to be AssignableFrom-related in any way, right?

@kumpera

This comment has been minimized.

Member

kumpera commented Mar 1, 2018

@lambdageek the problem is how we implemented Array::GetEnumerator for types that have interfaces. Say string.

We need to be able to do IEnumerable<ICloneable>::GetEnumerator() and IEnumerable<IComparable>::GetEnumerator() against a string[]. Our implementation kinda spits a single weird Enumerator object.

The right way would be to spit one type per IEnumerable that we need. Just like coreclr does.
I strongly agree with you that IEnumerable must stop being a magic interface.

@lambdageek

This comment has been minimized.

Member

lambdageek commented Mar 1, 2018

Ah. Thanks @kumpera. Interfaces were the piece that I was missing.

Did you mean that IEnumerator should stop being magic or IEnumerable? (For this bug it's just IEnumerator that's a problem - but in general I agree that both should stop being magic).

Did we ever try doing it the coreclr way? Was there an unacceptable regression in memory or AOT footprint?

@kumpera

This comment has been minimized.

Member

kumpera commented Mar 1, 2018

IEnumerator is magic cuz we fabricate one type per array type, instead of one per user-visible generic instance.

lambdageek added a commit to lambdageek/mono that referenced this issue Mar 7, 2018

[test] An array is not an IEnumerator`1
regression test for Regression test for mono#7093

lambdageek added a commit to lambdageek/mono that referenced this issue Mar 7, 2018

[metadata] An array is not an IEnumerator`1
Address mono#7093 in kind of an adhoc way.

The correct fix is to make
```
    ((IEnumerable<U>) (new T[] {...})).GetEnumerator()
```
return an `IEnumerator<U>` (like .NET does) instead of `IEnumerator<T>`
and to stop marking IEnumerator as an array special interface.

lambdageek added a commit to lambdageek/mono that referenced this issue Mar 8, 2018

[test] An array is not an IEnumerator`1
regression test for Regression test for mono#7093

lambdageek added a commit to lambdageek/mono that referenced this issue Mar 8, 2018

[metadata] An array is not an IEnumerator`1
Address mono#7093 in kind of an adhoc way.

The correct fix is to make
```
    ((IEnumerable<U>) (new T[] {...})).GetEnumerator()
```
return an `IEnumerator<U>` (like .NET does) instead of `IEnumerator<T>`
and to stop marking IEnumerator as an array special interface.

monojenkins added a commit to monojenkins/mono that referenced this issue Mar 8, 2018

An array is not an IEnumerator`1
regression test for Regression test for mono#7093

monojenkins added a commit to monojenkins/mono that referenced this issue Mar 8, 2018

An array is not an IEnumerator`1
Address mono#7093 in kind of an adhoc way.

The correct fix is to make
```
    ((IEnumerable<U>) (new T[] {...})).GetEnumerator()
```
return an `IEnumerator<U>` (like .NET does) instead of `IEnumerator<T>`
and to stop marking IEnumerator as an array special interface.

lambdageek added a commit to lambdageek/mono that referenced this issue Mar 8, 2018

[test] An array is not an IEnumerator`1
regression test for Regression test for mono#7093

lambdageek added a commit to lambdageek/mono that referenced this issue Mar 8, 2018

[metadata] An array is not an IEnumerator`1
Address mono#7093 in kind of an adhoc way.

The correct fix is to make
```
    ((IEnumerable<U>) (new T[] {...})).GetEnumerator()
```
return an `IEnumerator<U>` (like .NET does) instead of `IEnumerator<T>`
and to stop marking IEnumerator as an array special interface.

marek-safar added a commit that referenced this issue Mar 8, 2018

An array is not an IEnumerator`1
regression test for Regression test for #7093

marek-safar added a commit that referenced this issue Mar 8, 2018

An array is not an IEnumerator`1
Address #7093 in kind of an adhoc way.

The correct fix is to make
```
    ((IEnumerable<U>) (new T[] {...})).GetEnumerator()
```
return an `IEnumerator<U>` (like .NET does) instead of `IEnumerator<T>`
and to stop marking IEnumerator as an array special interface.

marek-safar added a commit that referenced this issue Mar 8, 2018

[test] An array is not an IEnumerator`1
regression test for Regression test for #7093

marek-safar added a commit that referenced this issue Mar 8, 2018

[metadata] An array is not an IEnumerator`1
Address #7093 in kind of an adhoc way.

The correct fix is to make
```
    ((IEnumerable<U>) (new T[] {...})).GetEnumerator()
```
return an `IEnumerator<U>` (like .NET does) instead of `IEnumerator<T>`
and to stop marking IEnumerator as an array special interface.

marek-safar added a commit that referenced this issue Mar 8, 2018

[test] An array is not an IEnumerator`1
regression test for Regression test for #7093

marek-safar added a commit that referenced this issue Mar 8, 2018

[metadata] An array is not an IEnumerator`1
Address #7093 in kind of an adhoc way.

The correct fix is to make
```
    ((IEnumerable<U>) (new T[] {...})).GetEnumerator()
```
return an `IEnumerator<U>` (like .NET does) instead of `IEnumerator<T>`
and to stop marking IEnumerator as an array special interface.

@marek-safar marek-safar closed this Mar 8, 2018

jonpryor added a commit to xamarin/xamarin-android that referenced this issue Mar 22, 2018

jonpryor added a commit to jonpryor/xamarin-android that referenced this issue Mar 24, 2018

jonpryor added a commit to xamarin/xamarin-android that referenced this issue Mar 24, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment