chapel-lang / chapel

a Productive Parallel Programming Language
https://chapel-lang.org
Other
1.79k stars 421 forks source link

Provide better error messages for `operator`s #24887

Open e-kayrakli opened 6 months ago

e-kayrakli commented 6 months ago

Here are some cases that can use better error messages:

1. proc type nature of operators could be incorporated into error messages

record myRec {
  var x: int;

  operator +(a) {return x+a; }
}

var r: myRec;

writeln(r+1);

says

error: unresolved call '+(myRec, 1)'

I'd like our compiler to remind me that an operator is a type method where the receiver is a type. I don't know how it thinks x is of type myRec either, but there must be some confusion about the receiver, I think

2. operators with wrong number of arguments simply aren't resolved and dropped

but they should cause error messages, because their existence probably indicates a not-so-innocent mistake in code that can turn up as more difficult errors:

record myRec {
  var x: int;

  // I wrote the following which resulted in unresolved call later, 
  operator +(a) {return a; }

  // ... b/c what I needed was:
  operator +(a, b:int) {return b; }

  // I could also write:
  operator +(a,b,c) {return a+b+c; }
}

var r: myRec;

writeln(r+1);

The first and the last methods there should have been caught by the compiler. I think they should be errors.

lydia-duncan commented 6 months ago

I'd like our compiler to remind me that an operator is a type method where the receiver is a type. I don't know how it thinks x is of type myRec either, but there must be some confusion about the receiver, I think

That looks like it's complaining about the line with the writeln, not the line inside the operator definition (which is where x is referenced). Basically, I think you're asking the resolution of operator calls to recognize that the operator name is the part that is more likely to be right as opposed to the number of arguments (which seems reasonable to me with you saying it now, but that might be me being biased towards the current confusion)

but they should cause error messages, because their existence probably indicates a not-so-innocent mistake in code that can turn up as more difficult errors

That'd involve us resolving code that is never called :) It's a little too difficult to do that now, but maybe dyno will make it less costly?

e-kayrakli commented 6 months ago

Oh, I see, you're right. So, maybe it all comes down to the same thing -- operators that are not qualified to be operators (because they have incorrect number of arguments, or something else like const lhs for += maybe?) should be reported as such. Because otherwise we get confusing errors.

That'd involve us resolving code that is never called :) It's a little too difficult to do that now, but maybe dyno will make it less costly?

That's a good point. FWIW, this is closer to a syntax error than a resolution error to me. I am totally game with waiting for dyno resolver to be online. But at the same time I wouldn't find a pre-resolution solution in the production resolver to be ugly. It could be done as an early sanity-check, potentially during scope resolution; or we can consider doing that while pruning unused operators, which may feel a bit awkward from a compiler architecture standpoint but could perform better.

P.S. thinking of cases where resolution may be necessary are things like operators with variadic arguments where number of actuals are constrained with a where or something. But that's a sci-fi story more than anything else. I would just disallow operators with variadic arguments.

lydia-duncan commented 6 months ago

I know operators already do unique things in the original resolution pass anyways, so making them even more unique seems reasonable to me. I can't remember if operator support has been started in dyno yet (I feel like I may have seen a task for it go by), but probably @mppf would know more

mppf commented 6 months ago

Yes we have some operator support

  1. operators with wrong number of arguments simply aren't resolved and dropped

we should be able to warn/error for such operators with adjustments to frontend/lib/uast/post-parse-checks.cpp

For (1), effort could go to the new dyno resolver or to the production resolver.