chapel-lang/chapel — pull request #250 by vasslitvinov (2014-08-28 18:46:37+00:00)
Title: 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 commit, 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.

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.

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.

* 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 commit 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 receiver calling 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 file 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 commit: coercionWrap() calls buildCoercionWrapper()
with this commit: 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.

* 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,
which is to be run under valgrind only for consistency of the 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 commit.

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