MiniZinc / libminizinc

The MiniZinc compiler
http://www.minizinc.org
Other
516 stars 81 forks source link

Overloading ++, show2d Error: evaluation error: index_set: wrong dimension #840

Closed CervEdin closed 1 month ago

CervEdin commented 2 months ago

I might be doing something wrong but I expected being able to overload ++ and use it in the following way.

array[int, int] of int: t1 = [|1, 2|];
array[int, int] of int: t2 = [|3, 4|5, 6|];

function array[int, int] of $T: append(
    array[int, int] of $T: tab1,
    array[int, int] of $T: tab2,
) = let {
  int: nrows = card(index_set_1of2(tab1)) + card(index_set_1of2(tab2))
} in assert(
  index_set_2of2(tab1) == index_set_2of2(tab2),
  "Column index sets must match \(index_set_2of2(tab1) != index_set_2of2(tab2))",
  array2d(
    1..nrows,
    index_set_2of2(tab1),
    array1d(tab1) ++ array1d(tab2)
  )
);

% Not sure I can overload ++ like this
function array[int, int] of $T: '++' (
  array[int, int] of $T: tab1,
  array[int, int] of $T: tab2,
) = append(tab1, tab2);

array[int, int] of int: t3 = t1 ++ t2;

output join("\n", [
  show2d(append(t1, t2)), % <-- works
  show(t1 ++ t2),         % <-- works
  % show2d(t1 ++ t2),     % <-- fails "Error: evaluation error: index_set: wrong dimension"
  show(t3),
  show(index_set_1of2(t3)),
  % show(index_set_2of2(t3)), % <-- fails "Error: evaluation error: index_set: wrong dimension"
]);

Can I overload ++ like this? Do I need to change the declaration of the overload or is this a bug?

cyderize commented 2 months ago

At the moment the ++ is treated as a special builtin and so the overload gets ignored, although it wouldn't be too difficult to allow overloading to work here. It's probably better to just use your own function though - if you add a 1D overload to your append function that will work correctly.

The line array[int, int] of int: t3 = t1 ++ t2; doesn't give any error, but ends up assigning the 1D array from the builtin ++ operator to the declaration, which is a bug - so we need to either support overloading here, or give a proper type error saying that the overload is not allowed.

CervEdin commented 2 months ago

I'm split on this.

The reason I was experimenting with this is because I find that t1 ++ t2 reads better, and is easier to type for most, than t1 `append` t2 or append(t1, t2) and operator overloading can make for really nice, readable models (like in OscaR).

But if it gets messy, I get why overloading ++ isn't supported.

t's probably better to just use your own function though - if you add a 1D overload to your append function that will work correctly.

Sorry, I'm not sure I follow what you meant here

cyderize commented 2 months ago

Yes, maybe that's a good argument for having proper user overloading - although I suppose the risk is that users could have different behaviours for the same overload of an operator which could make reading models confusing. If it really is obvious what an overloaded operator is supposed to do, maybe we should just add it to MiniZinc itself.

Sorry, I'm not sure I follow what you meant here

I just meant that if you did want to have two overloads for 1d/2d versions, then just calling it append would work fine as opposed to ++ - but you're right that append looks more cumbersome to read.

CervEdin commented 2 months ago

If it really is obvious what an overloaded operator is supposed to do, maybe we should just add it to MiniZinc itself.

I think my preferred solution here would be to have native support for concatenation of multi-dimensional arrays. Something along the lines of what my overload does would at least make sense to me for the language to support.