Where clause cannot filter functions with uint(?w) arguments

[mppf]

The where clause in the below program fails to limit the error cases produced by the compiler (namely generating real(8) for example). The program fails to compile with this error

d.chpl:5: error: illegal size 16 for real
d.chpl:5: error: illegal size 8 for real

proc fp754putZ(v : uint(?w)) : real(w)
   where w == 32 || w == 64
{
  var x:real(w) = v:real(w);
  writeln(v.type:string);
  writeln(x.type:string);
  return x;
}

proc test() {
  type R = real(32);
  type U = uint(numBits(R));
  const zero = 0:U;

  writeln(fp754putZ(zero));
}

test();

The error is being produced in normalize but whether the where clause applies can only be known in resolution.

[bradcray]

(pushed the wrong button, sorry...)

[bradcray]

I think that the current behavior is consistent with how we've defined the language to date, though I can understand why, intuitively, one would want something else to happen in this case. This suggests that we either need to clarify what's happening for such users (or retrain them) or change some aspect of the language definition. In any event, I don't think that "bug" is the right label here. I also think that the reference to uint in the issue name is misleading — this same behavior seems to exist if an int or bool is used as the formal/actual argument type.

Specifically, I believe that the role of where-clauses is currently to filter among existing function definitions, removing some from consideration; not to determine whether an instantiation of a generic function should be created or not. In addition, at present, when a size query like this is used on a formal of basic scalar type, I believe that the language says that we create all instantiations of that function for consideration, so the presence of the uint(?w) argument is going to cause us to create w=8, 16, 32, 64 versions of the function (with return types and local variables of type real(8) and real(16)) — and again, I don't think the where clause is meant to have any role in this instantiation action at present.

Under this interpretation, this suggests that to get this pattern to work, the author would need to be more precise about not injecting types that don't exist into the instantiated definition of the function. For example, one way to write this would be:

proc realType(param w) type {
  if w == 32 || w == 64 then
    return real(w);
  else  
    return void;
}

proc fp754putZ(v : uint(?w)) : realType(w)
   where w == 32 || w == 64
{
  var x:realType(w) = v:realType(w);
  writeln(v.type:string);
  writeln(x.type:string);
  return x;
}

proc test() {
  type R = real(32);
  type U = uint(numBits(R));
  const zero = 0:U;

  writeln(fp754putZ(zero));
}

test();

I was going to suggest that one way for us to make the original program work would be to only instantiate functions like this for the actual formal type sizes that they were called with. But while that approach would fix this specific test call, it wouldn't help with the case where the function was actually called with a uint(8) or uint(16) since the original signature would still try to create real(8) or real(16) expressions.

So then I think the only way to make the original code work would be to only instantiate functions whose where clauses (a) pass a "canResolve" style test (and silently drop those that can't on the floor?) and/or (b) provably have the potential to be true. This seems a little like skating on thin ice to me since what a given compiler can or cannot prove may vary depending on how aggressive it is in analyzing and optimizing expressions.

So, my intuition is to have the current behavior stand, but maybe there's some other way to make the original code work "as intended" that doesn't have the problems I'm seeing? In any case, I don't think the current behavior is awful in any way.

[bradcray]

I should add that even though I don't think it's a fix for this issue, I am generally open to pursuing the "only instantiate query types of basic scalar types for the sizes that they're called with" approach (so long as it doesn't change current test behavior), as I think it'd reduce compile-time significantly by reducing the number of unnecessary candidates that exist. One challenge to doing so can be seen in a case like this:

proc foo(x: int(?w) { ... }

foo(0);
foo(0:int(32));

Specifically, the presence of the int(64) instantiation created by the first call shouldn't be considered a solution to the int(32) instantiation desired by the second call even though an int(32) can coerce to int(64).

Historically, I think Vass had identified some similar challenge cases with binary int(?w) and int(w) arguments, but unless I'm misremembering, I think they were just variations on this same principle.

[bradcray]

Here's a related question that occurred to me after filing the previous comments. What should the following program do:

proc foo(x): unreal where false { writeln("In unreal foo"); }
proc foo(x): real where true { writeln("In real foo"); }

foo(10);

I think the two main answers are:

1) It should generate an error because there is no type named unreal 2) It should be OK because the where clause is false

I think the right answer is #1. Similarly, I think the following program should generate an error:

proc foo(x): real(8) where false { writeln("In foo"); }

foo(10);

since there is no type named real(8) (nor some other routine that resolves for that expression).

I view the original program as simply a slightly more complicated version of this program. So I increasingly think the current behavior is correct and appropriate.

[mppf]

@bradcray - The place where I think we're disagreeing is that I don't understand why the check that only legal integer/real/etc sizes are created can't happen within function resolution. (In other words, could we simply remove that check from the normalize pass and complete the resolution of integer types during function resolution, as with other generic types? Function resolution already has to handle resolving integer types because that's what's happen when you create int(p) where p is a param)

Generally speaking, our language offers an equivalent to C macros for conditional compilation with param conditionals and with where clauses. If I write a generic function with an if or a where clause that rules out invalid bit sizes, things work OK. But as you and I know, users who havn't read the manual generally expect proc f(x:int(?w)) to create a generic function. I don't understand why we need to interfere with that understanding if we don't have to. Even if we need to "stamp out" concrete versions of such functions for all integer widths, why does that need to happen in a way that precludes where clauses and param conditionals, which can work for concrete functions?

Right now, we can create actually generic versions of functions on all integer types with things like proc f(x:integral). These are just less documented and less natural than the int(?w) syntax. But why do these need to be so different?

Besides that, from a compiler-design point of view, it's always "wrong" for the normalizer to reason about types. Types aren't established until resolution. This just looks like yet another case of a misplaced type check.

How about a somewhat more concrete example, showing how minor changes might lead to a surprising failure to compile?

proc integralToReal(x:integral) {
  if numBits(x.type) == 64 || numBits(x.type) == 32 {
    writeln("OK");
    var r:real(numBits(x.type));
    writeln(r);
  } else {
    writeln("Not good");
  }
}

integralToReal(0:int(32));
integralToReal(0:int(8));

Works great, compiles, and runs.

proc intToReal(x:int(?w)) {
  if numBits(x.type) == 64 || numBits(x.type) == 32 {
    writeln("OK");
    var r:real(numBits(x.type));
    writeln(r);
  } else {
    writeln("Not good");
  }
}

intToReal(0:int(32));
intToReal(0:int(8));

Also works just fine, compiles and runs. Note that the int(?w) argument changed whether or not this function was "generic" and changed where in the compiler it's "stamped out". But that does not prevent it from working just the same as the previous one.

proc intToRealSimpler(x:int(?w)) {
  if w == 64 || w == 32 {
    writeln("OK");
    var r:real(w);
    writeln(r);
  } else {
    writeln("Not good");
  }
}

intToRealSimpler(0:int(32));
intToRealSimpler(0:int(8));

This one fails to compile, for the same reason as the program in the PR message. But if I literally comment out the USR_FATAL_CONT it hits, it compiles and runs just like the others. This indicates to me that the problem here is merely that such errors should be reported only during function resolution. Similarly, commenting out the error message allows the program in the PR message to work if I make the simple modification of dropping the return type declaration from fp754putZ.

I think that the issue about functions with an invalid declared return type and a where clause that evaluates to false is also interesting to think about, but it's not really what I was trying to bring up in this issue. (And I feel similarly about proposed changes to the "stamping out" of int(?w) arguments - interesting but not really the problem here).

[bradcray]

OK, so this clarifies things for me a bit (I think).

First, I agree that your last code example above ought to work and the fact that it doesn't is a bug. If the issue had opened with that example, we'd have been on the same page from the start.

I also feel unsettled that we're making any attempt to resolve types in normalize. Here's an attempt at a specific (yet artificial) example of why I think we have to wait until function resolution, putting what I think you were alluding to above into words:

proc returnWidth(x: int) param {
  return 32;
}
proc returnWidth(x: real) param {
  return 64;
}
var i: int(returnWidth(2.4));

I know that @noakesmichael did some changes along these lines recently (moving resolution for basic types earlier) and it looks like the FATAL that we're hitting in this case was last touched by Mike and @benharsh (who changed it from a FATAL to a FATAL_CONT). These changes made me nervous at the time for the reason you suggest (that there will always be cases where we need to resolve such types at function resolution time, so what do we gain by resolving some of them earlier?) I'm sure there was a motivation, but can't recall offhand whether it was reduced compilation time, a way to fix or work around some other bug or compiler architecture issue, or a way to temporarily shift our weight off of bad code which we could now shift back away from. I didn't dig into the logs to help refresh my memory in writing this up.

Beyond that issue (which I agree is a compiler bug), I still think that the original example posed in this issue has more of a language design air to it for the following reason: Without looking at the compiler code or your latest example, it seems clear to me that the return type expression of the function is being resolved by the compiler prior to the evaluation of the where-clause, and this didn't strike me as being surprising or unnatural. In part this is because it's part of the function's prototype/definition (rather than its body, say), and in part it's because it syntactically appears before the where clause. So it seems like in any world where we continue to stamp out all bit widths of functions like this, we will run into this error as long as that assumption is true (we should resolve return expressions prior to where clauses). This is also why the "broken return type" examples with false where-clauses in my previous examples seemed like they should generate errors to me.

I suppose an alternate viewpoint would be that since function resolution is only considering the argument types, not the return type, we should not bother trying to resolve the return type expression until we feel confident that the where clause isn't removing the candidate from consideration. This might be your current view, I'm not certain.

In any case, I think it's a language design issue in that it requires specifying the evaluation order of the return type expression and the where clause, and/or whether or not we should stamp out all bitwidths for cases like this.

[bradcray]

(I should note that it may also be the case that Mike was just refactoring existing code... I thought that he put in some effort to do resolution of certain types earlier in the compiler, though, which is why I characterized it that way. Mike, can you help us out?)

[noakesmichael]

On Nov 30, 2017, at 11:33 AM, Brad Chamberlain wrote:

(I should note that it may also be the case that Mike was just refactoring existing code... I thought that he put in some effort to do resolution of certain types earlier in the compiler, though, which is why I characterized it that way. Mike, can you help us out?)

I think it’s hard to characterize the effort in a way that is clear and objective. There are many possible views of how code and business logic is, or should be, distributed across certain passes.

Interactions between, say, scope-resolve, normalize, and function-resolution are particularly challenging to reason about.

So some of how I characterize this work will reflect personal ways of simplifying these complex issues.

If I understand and recall correctly, the work that is being referenced occurred during early work on “initializers”. Historically, the handling of variable initialization has been treated as two operations

1) default initialize

2) assign.

Consider

var x = 10;

or

var r = new MyConcreteRecord(10, 20);

In both cases it is clear what the type of ‘x’ and ‘’r’ is. But normalize obstinately refused to acknowledge this and emitted generalized AST that included the introduction of PRIM_INIT and then assignment in ways that complicated understanding the type for function resolution.

The overall impact of these machinations were limited for primitive types, later passes arranged to generate the natural C code, but was more challenging for record init.

The response to these problems was to explore the idea of performing “local type inference” in certain simple cases like the ones above. I believe that was correct and useful and I think we should push more in this direction when warranted.

I think it is fair to call this “refactoring” of existing code. For one use case it was appropriate to expose some “private” logic that was/is used in function resolution so that it could also be applied in normalize.

Be clear though that the general case of

var x = myFunctionThatRetunsAnInt(20);

continues to be handled as it was before.

Hope this helps, makes sense, and is accurate.

[bradcray]

Thanks Mike. Cutting to the chase, do you agree that this example from Michael above is currently a bug on master:

proc intToRealSimpler(x:int(?w)) {
  if w == 64 || w == 32 {
    writeln("OK");
    var r:real(w);
    writeln(r);
  } else {
    writeln("Not good");
  }
}

intToRealSimpler(0:int(32));
intToRealSimpler(0:int(8));

(I can argue why I think it is if it isn't obvious).

Extrapolating from what you guys have said, my guess is that we need to not have the USR_FATAL_CONT() get triggered when we're calling the shared code from normalize (where it's currently firing too conservatively because the code it's normalizing may never get executed, as in this case), but to trigger it when it's encountered in the resolution pass. Seems like it should be a fairly simple change and help Michael make forward progress. I'm just hoping to get agreement that it's the right thing to do at which point we can determine who should own it.

[noakesmichael]

Here’s an even simpler version that exposes the current bug.

proc intToRealSimpler(x : int(?w)) { if w == 64 || w == 32 { var r : real(w);

writeln(r);

} }

I’d be happy to fix this in the way that Brad suggested.

[noakesmichael]

Sorry that that wasn’t as clear as I intended. The point here is that the normalization that breaks this complete program occurs whether or not there are any calls to this procedure.

The expansion of procedures with parameterized primitive types occurs early in normalize and then every variation of each procedure is normalized.

[bradcray]

I’d be happy to fix this in the way that Brad suggested.

Thanks—I've already got a fix implemented (and would have testing running except that my test-venv seems to be out-of-date... :( ).

[bradcray]

Sorry that that wasn’t as clear as I intended. The point here is that the normalization that breaks this complete program occurs whether or not there are any calls to this procedure.

That's a good point—I was wondering if that would be the case. I'll add this test as well if my patch flies (and hand it off to someone else if it doesn't fly and I can't keep working on it tomorrow... I needed an end-of-the-day break).

[noakesmichael]

I am confident that your work will fix this bug. This is the best example of a “5 min-fix” I’ve seen in a while. It’s not quite a “one line” change but it’s close.

In the unlikely case that I’ve jinxed you and you have to time out I’ll be happy to finish up.

[bradcray]

You were overly optimistic, Mike. :) The patch doesn't make anything "incorrect" but changes the behavior for two tests (arguably for the worse) by replacing early normalization-time error messages with resolution-time ones (as a result of hitting them earlier than the routine in question I've got to think, but haven't had a chance to look into it yet).

My attempt is here: PR #7909 I don't expect to work on it more tonight.

[noakesmichael]

My understanding is that Michael is blocked by this tiny issue. I’ll complete the work and merge.

[mppf]

hi @noakesmichael and @bradcray - thanks for your help resolving it & I'm happy to see the progress. However I'm not actually blocked by this issue (I'm more blocked by the promotion/default argument discussion). This one is just an example that came out of a recent discussion on chapel-developers.

[mppf]

The issue I intended to write about in this issue has been addressed, but the original example does not work (because it resolves the return type eagerly, before the where clause is evaluated). Anyway, I'll close the issue.