Skip to content
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

Object slicing can bite our arses #7637

Closed
siliconvoodoo opened this issue Apr 17, 2018 · 2 comments
Closed

Object slicing can bite our arses #7637

siliconvoodoo opened this issue Apr 17, 2018 · 2 comments

Comments

@siliconvoodoo
Copy link

siliconvoodoo commented Apr 17, 2018

As mentioned in the forum https://forum.nim-lang.org/t/3760 I'm shadowing the report in the tracker for better task management opportunities. (feel free to close this if redundant or otherwise judged).

I am not blaming Nim on this one, because as a C++ veteran, I expect this behavior among the possibilities, and I kind of like it being opened to use (in the sense of don't impose too much restriction on style too early on. Or "ask for forgiveness not permission" sense.)

BUT, a warning would be very productive :)

type
  Fruit = object of RootObj
    name*: string
  Apple = object of Fruit
  Pear = object of Fruit

method eat(f: Fruit) {.base.} =
  raise newException(Exception, "PURE VIRTUAL CALL")

method eat(f: Apple) =
  echo "fruity"

method eat(f: Pear) =
  echo "juicy"

let basket = [Apple(name:"a"), Pear(name:"b")]

eat(basket[0])

My reason to be taken aback and surprised, was that Nim feels like a scripting language, and this kind of mistakes are not possible in usual script langs. So I don't know how to feel, and I can't judge. I'm just reporting.

Let's take C#, this would work in C# with struct (the C# value-type, so Nim's equivalent of non-ref objects). Probably because the array would hold references to the objects (in the form of the type of the interface they implement), even if they are allocated on the stack space.
proof of this concept:
https://ideone.com/CXVHQ1
But note that changing to type inference
var basket = new[]{a, p};
gives

error CS0826: The type of an implicitly typed array cannot be inferred from the initializer. Try specifying array type explicitly

A very friendly message that gives you the solution immediately.

Half related: a great talk about memory placement of value types:
https://blogs.msdn.microsoft.com/ericlippert/2010/09/30/the-truth-about-value-types/

In C++ it's not a problem either, it would simply employ a very explicit syntax. Where objects are, and what type the array holds, is in the code, not the spec. [Of course the drawback is that you are not protected against leaking the pointers out of this stack scope and get dangling pointers. Though valgrind will catch it.]

	struct Fruit { string name; virtual void eat() { throw "ouch"; } };
	struct Apple : Fruit { void eat() { printf("fruity"); } };
	struct Pear : Fruit { void eat() { printf("juicy"); } };
	
	Apple a;
	Pear p;
	Fruit* basket[] = {&a, &p};
	
	basket[0]->eat();

https://ideone.com/PaOkTN

note that (akin to C#), C++ does not want to infer the base type in case of non consistent array element's types:
if I write;
auto basket = {&a, &p};

it says:

error: unable to deduce ‘std::initializer_list’ from ‘{(& a), (& p)}’
auto basket = {&a, &p};
note: deduced conflicting types for parameter ‘auto’ (‘main()::Apple*’ and ‘main()::Pear*’)

And if you store plain Fruit like in my original Nim snippet, it "object slices" just the same. with no warning either.
https://ideone.com/hD2dPg

@dom96
Copy link
Contributor

dom96 commented Apr 17, 2018

If you use ref for all types it works. Perhaps we should disallow methods for non-ref types? :)

@Araq
Copy link
Member

Araq commented Apr 17, 2018

We should really fix this one for the next release. Calling a method is fine but the object slicing behaviour is just a bug that I never fixed.

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

No branches or pull requests

4 participants