Compiler: do not build coercion wrappers.
Followed up on Tom's reviewer comments.
Merge pull request #250 from vasslitvinov/coercion-wrapper-elimination

Compiler: do not build coercion wrappers.

This follows the lead of #121 that eliminated (re)order wrappers.

The compiler used to build a "coercion wrapper" during resolution when
there is a type mismatch between a formal and an actual in a call
and some transformation, or "coercion," needs to happen.

For example:
* an "implicit conversion" prescribed by the spec needs to happen
* something specific to the internal representation esp.
  the actual needs to be dereferenced
* if it's a sync or single variable, it needs to be readFE etc.

With this merge, if coercion(s) are needed, the compiler
inserts them immediately preceeding the CallExpr,
instead of creating a wrapper and putting them there.

This replaces coercionWrap() with coerceActuals().
Since I needed to access resolveCall() and resolveFns() from
wrappers.cpp, I made them non-static.

  compiler/resolution/resolution.h
  compiler/resolution/functionResolution.cpp
  compiler/resolution/wrappers.cpp



IMPLEMENTATION NOTES
  aka some thorns I had on this, seemingly straightforward, path


* For a given (actual, formal) pair, more than one transformation
may be needed. For example, dereference + implicit conversion int->real.
The old code seemlessly (pure magic, I swear!) created multiple wrappers
invoking one another. See Tom's note on magic below (1).

I had to loop around adding a coercion until they are not needed any more,
i.e. the types of the (sufficiently coerced) actual and the formal match
or are sufficiently close.

See the comment for 'checksLeft' in coerceActuals() for more details
and Tom's note below (2).

This test (not modified) requires an above-average checksLeft limit:
studies/590o/blerner/xml-parse.chpl


* If a call that needed coercions got removed later in postFold(), e.g.
if the callee is a param function, the added coercions would be removed too.
That's because they'd be in a wrapper and the wrapper would be removed.

This is noticeable when the actuals are sync/single variables and
they are read/written (readFE etc.) into the formals.

I am not mimicking this behavior - because coercionWrap()/coerceActuals()
executes *before* we know for sure whether the call will be removed.

Instead, I never insert coercions for 'param' functions
and always insert coercions for 'type' functions.
I put a comment in coerceActuals() to this effect.

I will add the test separately:

  test/functions/vass/type-and-param-function-arg-conversions.chpl

to record this behavior.
Note: this has not been discussed/accepted by the group.
See also Tom's note (3).


* I had to fix refNecessary() for cullOverReferences(), so that
record-wrapped types are still considered "needing a reference" even
when they undergo a dereference. Why it worked before is also pure magic.

Well it did not worked quite perfectly, and the .good for one of the tests
was incorrect: when an array was passed by blank intent, it was considered
"not needing a reference". This was due to the fact that, in this case,
the corresponding _array record would be passed to the wrapper
by 'const in' intent. So the .good for one of the tests was incorrect,
I corrected it:

  test/functions/deitz/test_setter_method4.good

Conversely, my implementation considers an array always
"needing a reference", so the behavior is still not perfect.
I special-case coercion temporaries for that, hence the new flag

  FLAG_COERCE_TEMP

  compiler/include/flags_list.h
  compiler/resolution/cullOverReferences.cpp

The details will be in a .future that I will add separately:

  test/functions/vass/setter-passed-to-another-fun.chpl


* Another semantic issue I came across is whether the value of a
sync/single variable be read when it is the 'this' argument
and the callee is a method on the underlying type.

The compiler currently special-cases the receiver case, bypassing
synchronization aka readFE. I believe readFE should be performed, and
I will add a feature-request future separately:

  test/functions/vass/sync-to-this-of-underlying-type.chpl


* I added explicit creation of destructors for classes that implement
first-class functions. Before the destructors on 'object' would be
invoked.

The motivation comes from the "resolve destructor" portion of resolveFns().
Invoking a destructor on a object would cause a coercion
from the fcf class to the object superclass to get inserted prior
to 'call', and that coercion would not get deleted.
See the two added calls to buildDefaultDestructor().

  compiler/include/passes.h
  compiler/passes/buildDefaultFunctions.cpp
  compiler/resolution/functionResolution.cpp

Instead of adding code to delete the coercion, I opted to add
destructors for the subclass directly so the coercions are not needed.
My motivation primarily was that the code for the extra deletions would
be inelegant. However, we can go that way instead, it's quite straightforward.

While there, I added some assertions and removed the "new CallExpr(call)"
wrapper around 'call', which is already a CallExpr.

  compiler/resolution/functionResolution.cpp


* Notes on addArgCoercion()

 - I renamed a couple of functions:
   before this merge:   coercionWrap()  calls  buildCoercionWrapper()
   with this merge:     coerceActuals()  calls  addArgCoercion()

 - In addArgCoercion(), the only logic I changed is removing
   the case when hasFlag(FLAG_INSTANTIATED_PARAM).
   That case never happens now, so I added an assertion instead.

   The remaining changes are due to implementation details and formatting.

  compiler/resolution/wrappers.cpp


* I came across an idiom that corrupts memory, with my changes.
We do not exercise that idiom in our current code base.
I feel it will be easier to fix it after we get a better grip on
reference counting and representing references, esp. to arrays.

Meanwhile, with Brad's approval, I am recording this idiom as a bug future.
I run it under valgrind only - for consistency of the .bad output.

  .gitignore
  test/arrays/vass/setter-assigned-to-a-var.chpl



CHANGES TO EXISTING TESTS

* --print-callstack-on-error now does not print some wrapper
stack frames. Don't know why it did before.

  test/execflags/vass/print-callstack-on-error-1.good
  test/execflags/vass/print-callstack-on-warning-1.good

* In the test solver-blc.chpl, each of the lines 64, 65, 66, in isolation
from the other two, generates a warning - regardless of this merge.

Taken together, the three lines generate one warning before my changes
and two warnings with my changes. I think this is due to peculiarities
of our warning cache and this change is acceptable.

  test/studies/590o/kfm/solver-blc.good



TOM'S NOTES

(1). The magic associated with multiple coercion transformations
[before this merge] is easily explained.  The process of resolving a
call is merely to select the function that is being called.  Resolving
the body of that function is a separate step.  When a wrapper function
is created, it contains an unresolved call that is again subjected to
the four "adjustments" (default argument substitution, reordering,
coercion and promotion) that were applied to the original call.

(2). I think the multiple coercions (meaning more than two) is an
artifact of the special handling of sync variable.  The C++ spec
specifically limits the number of coercions that can be applied --
probably recognizing that without such a limit it would be fairly easy
to introduce hard-to-explain behavior (examples provided upon
request).  For now, it probably makes good sense to stick close to the
C++ model.
     Moving the special handling of sync demotion (i.e. extracting the
underlying value from a sync variable) into module code would render
this special-case code in the compiler moot and provide a more regular
solution.

(3). Ultimately, I hope to push all code related to sync variable
implementation into module code.  In which case, different behavior
associated with a sync based on whether it is evaluated as a param or
at run time will reside entirely within module code.