ClojureScript

cljs.core/IFn implementations generate duplicated code

Details

  • Type: Defect Defect
  • Status: Open Open
  • Priority: Major Major
  • Resolution: Unresolved
  • Affects Version/s: None
  • Fix Version/s: None
  • Component/s: None
  • Labels:
    None
  • Patch:
    Code and Test

Description

Currently when defining types that implement cljs.core/IFn a protocol fn is generated as well as a .call method that repeats the entire code instead of dispatching to the generated protocol fn.

(deftype InvokeTest [a b]
  IFn
  (-invoke [_]
    (+ a b)))
cljs.user.InvokeTest.prototype.cljs$core$IFn$_invoke$arity$0 = function() {
  var self__ = this;
  var _ = this;
  return self__.a + self__.b;
};

.call repeats all code instead of calling this.cljs$core$IFn$_invoke$arity$<n>. Multiple arities repeat the code for each arity.

cljs.user.InvokeTest.prototype.call = function(self__) {
  var self__ = this;
  var self____$1 = this;
  var _ = self____$1;
  return self__.a + self__.b; 
};

Should be something like

cljs.user.InvokeTest.prototype.call = function(self__) {
  switch(arguments.length) {
    case 0:
      return this.cljs$core$IFn$_invoke$arity$0();
    ...
    default:
      throw new Error("Invalid arity: " + arguments.length);
  }
};

Activity

Hide
Thomas Heller added a comment -

Note that Closure does not remove the .call functions in :advanced

Pseudo-names output:

$JSCompiler_prototypeAlias$$ = $cljs$core$Keyword$$.prototype;
$JSCompiler_prototypeAlias$$.call = function() {
  var $G__41215$$ = null;
  $G__41215$$ = function($G__41215$$, $coll$jscomp$188$$, $not_found$jscomp$10$$) {
    switch(arguments.length) {
      case 2:
        return $cljs$core$get$$.$cljs$core$IFn$_invoke$arity$2$($coll$jscomp$188$$, this);
      case 3:
        return $cljs$core$get$$.$cljs$core$IFn$_invoke$arity$3$($coll$jscomp$188$$, this, $not_found$jscomp$10$$);
    }
    throw Error("Invalid arity: " + (arguments.length - 1));
  };
  $G__41215$$.$cljs$core$IFn$_invoke$arity$2$ = function($G__41215$$, $coll$jscomp$186$$) {
    return $cljs$core$get$$.$cljs$core$IFn$_invoke$arity$2$($coll$jscomp$186$$, this);
  };
  $G__41215$$.$cljs$core$IFn$_invoke$arity$3$ = function($G__41215$$, $coll$jscomp$187$$, $not_found$jscomp$9$$) {
    return $cljs$core$get$$.$cljs$core$IFn$_invoke$arity$3$($coll$jscomp$187$$, this, $not_found$jscomp$9$$);
  };
  return $G__41215$$;
}();
$JSCompiler_prototypeAlias$$.$cljs$core$IFn$_invoke$arity$1$ = function($coll$jscomp$189$$) {
  return $cljs$core$get$$.$cljs$core$IFn$_invoke$arity$2$($coll$jscomp$189$$, this);
};
$JSCompiler_prototypeAlias$$.$cljs$core$IFn$_invoke$arity$2$ = function($coll$jscomp$190$$, $not_found$jscomp$11$$) {
  return $cljs$core$get$$.$cljs$core$IFn$_invoke$arity$3$($coll$jscomp$190$$, this, $not_found$jscomp$11$$);
};
Show
Thomas Heller added a comment - Note that Closure does not remove the .call functions in :advanced Pseudo-names output:
$JSCompiler_prototypeAlias$$ = $cljs$core$Keyword$$.prototype;
$JSCompiler_prototypeAlias$$.call = function() {
  var $G__41215$$ = null;
  $G__41215$$ = function($G__41215$$, $coll$jscomp$188$$, $not_found$jscomp$10$$) {
    switch(arguments.length) {
      case 2:
        return $cljs$core$get$$.$cljs$core$IFn$_invoke$arity$2$($coll$jscomp$188$$, this);
      case 3:
        return $cljs$core$get$$.$cljs$core$IFn$_invoke$arity$3$($coll$jscomp$188$$, this, $not_found$jscomp$10$$);
    }
    throw Error("Invalid arity: " + (arguments.length - 1));
  };
  $G__41215$$.$cljs$core$IFn$_invoke$arity$2$ = function($G__41215$$, $coll$jscomp$186$$) {
    return $cljs$core$get$$.$cljs$core$IFn$_invoke$arity$2$($coll$jscomp$186$$, this);
  };
  $G__41215$$.$cljs$core$IFn$_invoke$arity$3$ = function($G__41215$$, $coll$jscomp$187$$, $not_found$jscomp$9$$) {
    return $cljs$core$get$$.$cljs$core$IFn$_invoke$arity$3$($coll$jscomp$187$$, this, $not_found$jscomp$9$$);
  };
  return $G__41215$$;
}();
$JSCompiler_prototypeAlias$$.$cljs$core$IFn$_invoke$arity$1$ = function($coll$jscomp$189$$) {
  return $cljs$core$get$$.$cljs$core$IFn$_invoke$arity$2$($coll$jscomp$189$$, this);
};
$JSCompiler_prototypeAlias$$.$cljs$core$IFn$_invoke$arity$2$ = function($coll$jscomp$190$$, $not_found$jscomp$11$$) {
  return $cljs$core$get$$.$cljs$core$IFn$_invoke$arity$3$($coll$jscomp$190$$, this, $not_found$jscomp$11$$);
};
Hide
David Nolen added a comment -

Just leaving a note here that the above example looks deceivingly like what is being suggested, but it is, as noted by Thomas, dispatching to `get`.

Show
David Nolen added a comment - Just leaving a note here that the above example looks deceivingly like what is being suggested, but it is, as noted by Thomas, dispatching to `get`.
Hide
Thomas Heller added a comment - - edited

Attached patch adjusts the generated .call fn as planned.

The generated code now is more compact and does not repeat.

$JSCompiler_prototypeAlias$$ = $cljs$core$Keyword$$.prototype;
$JSCompiler_prototypeAlias$$.call = function($unused__60227__auto__$jscomp$3$$) {
  switch(arguments.length - 1) {
    case 0:
      return this.$cljs$core$IFn$_invoke$arity$0$();
    case 1:
      return this.$cljs$core$IFn$_invoke$arity$1$(arguments[1]);
    case 2:
      return this.$cljs$core$IFn$_invoke$arity$2$(arguments[1], arguments[2]);
    default:
      throw Error(["Invalid arity: ", $cljs$core$str$$.$cljs$core$IFn$_invoke$arity$1$(arguments.length - 1)].join(""));
  }
};
$JSCompiler_prototypeAlias$$.$cljs$core$IFn$_invoke$arity$1$ = function($coll$jscomp$183$$) {
  return $cljs$core$get$$.$cljs$core$IFn$_invoke$arity$2$($coll$jscomp$183$$, this);
};
$JSCompiler_prototypeAlias$$.$cljs$core$IFn$_invoke$arity$2$ = function($coll$jscomp$184$$, $not_found$jscomp$7$$) {
  return $cljs$core$get$$.$cljs$core$IFn$_invoke$arity$3$($coll$jscomp$184$$, this, $not_found$jscomp$7$$);
};

I'm unsure why the self-host tests fail.

Show
Thomas Heller added a comment - - edited Attached patch adjusts the generated .call fn as planned. The generated code now is more compact and does not repeat.
$JSCompiler_prototypeAlias$$ = $cljs$core$Keyword$$.prototype;
$JSCompiler_prototypeAlias$$.call = function($unused__60227__auto__$jscomp$3$$) {
  switch(arguments.length - 1) {
    case 0:
      return this.$cljs$core$IFn$_invoke$arity$0$();
    case 1:
      return this.$cljs$core$IFn$_invoke$arity$1$(arguments[1]);
    case 2:
      return this.$cljs$core$IFn$_invoke$arity$2$(arguments[1], arguments[2]);
    default:
      throw Error(["Invalid arity: ", $cljs$core$str$$.$cljs$core$IFn$_invoke$arity$1$(arguments.length - 1)].join(""));
  }
};
$JSCompiler_prototypeAlias$$.$cljs$core$IFn$_invoke$arity$1$ = function($coll$jscomp$183$$) {
  return $cljs$core$get$$.$cljs$core$IFn$_invoke$arity$2$($coll$jscomp$183$$, this);
};
$JSCompiler_prototypeAlias$$.$cljs$core$IFn$_invoke$arity$2$ = function($coll$jscomp$184$$, $not_found$jscomp$7$$) {
  return $cljs$core$get$$.$cljs$core$IFn$_invoke$arity$3$($coll$jscomp$184$$, this, $not_found$jscomp$7$$);
};
I'm unsure why the self-host tests fail.
Hide
Thomas Heller added a comment -

Not a big deal but the cljs.core code size goes from 168.8KB to 163.2KB (non-gzip) in my :advanced test build so it definitely reduces the amount of code generated overall.

Show
Thomas Heller added a comment - Not a big deal but the cljs.core code size goes from 168.8KB to 163.2KB (non-gzip) in my :advanced test build so it definitely reduces the amount of code generated overall.
Hide
Thomas Heller added a comment -

Updated patch fixes bad implementation using max fixed arity assuming that all lower arities would always be present.

Also fixed all self-host issues and tests seem to pass fine now.

Show
Thomas Heller added a comment - Updated patch fixes bad implementation using max fixed arity assuming that all lower arities would always be present. Also fixed all self-host issues and tests seem to pass fine now.
Hide
Thomas Heller added a comment - - edited

Correction: The test for CLJS-2133 currently fails.

ERROR in (test-cljs-2133) (Error:NaN:1)
Uncaught exception, not in assertion.
expected: nil
  actual: #object[Error Error: Invalid arity: 1]

I can adjust the patch to allow the variadic behavior but as it is officially not supported and generates a warning since 1.9.660 it may be time to remove support entirely?

Show
Thomas Heller added a comment - - edited Correction: The test for CLJS-2133 currently fails.
ERROR in (test-cljs-2133) (Error:NaN:1)
Uncaught exception, not in assertion.
expected: nil
  actual: #object[Error Error: Invalid arity: 1]
I can adjust the patch to allow the variadic behavior but as it is officially not supported and generates a warning since 1.9.660 it may be time to remove support entirely?
Hide
Mike Fikes added a comment - - edited

We've had a diagnostic in place via CLJS-2134 since 1.9.660.

Comment from David in Slack:

"I think 1.9.660 is probably long enough go to not be too concerned about it
I'm ok with the patch and we'll see what the feedback is"

Perhaps we can just remove the unit test that was added with CLJS-2133 as part of the patch for this ticket?

Show
Mike Fikes added a comment - - edited We've had a diagnostic in place via CLJS-2134 since 1.9.660. Comment from David in Slack: "I think 1.9.660 is probably long enough go to not be too concerned about it I'm ok with the patch and we'll see what the feedback is" Perhaps we can just remove the unit test that was added with CLJS-2133 as part of the patch for this ticket?

People

Vote (0)
Watch (2)

Dates

  • Created:
    Updated: