Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure unboundFunction is bound to the right symbol #439

Merged
merged 3 commits into from
Sep 1, 2022

Conversation

davazp
Copy link
Member

@davazp davazp commented Aug 25, 2022

That way the error messagae will be correct regardless if we invoke it
like symbol.fvalue() or if we assign it first into a variable, losing
the this context.

Detailed

I started debugging #438, narrow it down to this simple issue:

(search "a" "b" :test 'equalp)
; ERROR: Cannot read properties of undefined (reading 'name')

It should be more intuitive than that error. Like the regular equalp is not bound.

Looking at the definition, and how it dispatches by sequence type, I tried with other couple of sequences:

jscl/src/sequence.lisp

Lines 353 to 383 in 29885f7

(defun mismatch (sequence1 sequence2 &key key (test #'eql testp) (test-not nil test-not-p)
(start1 0) (end1 (length sequence1))
(start2 0) (end2 (length sequence2)))
(let ((index1 start1)
(index2 start2))
(while (and (<= index1 end1) (<= index2 end2))
(when (or (eql index1 end1) (eql index2 end2))
(return-from mismatch (if (eql end1 end2) NIL index1)))
(unless (satisfies-test-p (elt sequence1 index1) (elt sequence2 index2)
:key key :test test :testp testp
:test-not test-not :test-not-p test-not-p)
(return-from mismatch index1))
(incf index1)
(incf index2))))
(defun list-search (sequence1 list2 args)
(let ((length1 (length sequence1))
(position 0))
(while list2
(let ((mismatch (apply #'mismatch sequence1 list2 args)))
(when (or (not mismatch) (>= mismatch length1))
(return-from list-search position)))
(pop list2)
(incf position))))
(defun vector-search (sequence1 vector2 args)
(let ((length1 (length sequence1)))
(dotimes (position (length vector2))
(let ((mismatch (apply #'mismatch sequence1 (subseq vector2 position) args)))
(when (or (not mismatch) (>= mismatch length1))
(return-from vector-search position))))))

CL-USER> (search '(1) '(2) :test 'equalp)
ERROR: Cannot read properties of undefined (reading 'name')
CL-USER> (search #(1) #(2) :test 'equalp)
ERROR: Cannot read properties of undefined (reading 'name')

still happens. So it seems that it migth be in the generic util satisfies-test-p, and indeed:

CL-USER> (in-package :jscl)
#<PACKAGE JSCL>
JSCL> (satisfies-test-p 1 2)
NIL
JSCL> (satisfies-test-p 1 2 :test 'equalp)
ERROR: Cannot read properties of undefined (reading 'name')

The only thing this does is calling funcall

So we can simplify it even more to just funcall:

CL-USER> (funcall 'f)
ERROR: Cannot read properties of undefined (reading 'name')

At this point this is a primitive compilation defined in the compiler. I did generate the code for that simple example with

(jscl::with-compilation-environment
  (jscl::compile-toplevel '(funcall 'f)))

resulting in:

var l1=internals.intern('F','CL-USER');
(function(){
  var F=l1;
  return (typeof F==='function'?F:F.fvalue)(internals.pv);
})();

Not a lot happening.. no mention of the name. Except I noticed something. symbol.fvalue is initialized to unboundFunction in order to generate an error if a undefined function is called. Code code in prelude is

jscl/src/prelude.js

Lines 334 to 336 in 29885f7

internals.unboundFunction = function () {
throw new Error("Function '" + this.name + "' undefined");
};

This could explain. If unboundFunction's this is only defined if we call it like symb.fvalue. If we get that value out into another expression and call it. this will be undefined.

We can try that int he JS console:

jscl.internals.intern('F').fvalue()
/*
jscl.js:337 Uncaught Error: Function 'F' undefined
    at internals.unboundFunction [as fvalue] (jscl.js:337:9)
    at <anonymous>:1:30
*/

that is correct. But if the compiler generates:

(true? jscl.internals.intern('F').fvalue: null)()
/*
jscl.js:337 Uncaught TypeError: Cannot read properties of undefined (reading 'name')
    at internals.unboundFunction (jscl.js:337:39)
    at <anonymous>:1:48
internals.unboundFunction @ jscl.js:337
(anonymous) @ VM984:1
*/

Then we can reproduce the problem.

That is kind of what the compiler generates!

(typeof F==='function'?F:F.fvalue)(internals.pv);

Possible solutions for this would be:

  • Not using this in unboundSymbol, bind it it to the symbol when the symbol is interned
  • Changing the generated code to make sure we call the function with the right this context associated.

We could generate fo rinstance

(typeof F==='function'?F:F.fvalue).call(F, internals.pv);

The bind method is more robust probably.

@davazp davazp mentioned this pull request Aug 25, 2022
@mmontone
Copy link

mmontone commented Aug 25, 2022

I think this patch introduces the problem that you get functions for nonexistent things using the function reader macro:

For example, if I do:

#'asdf

I get <FUNCTION> at the repl. And only non-existent function error when I try to call it.

@davazp
Copy link
Member Author

davazp commented Aug 25, 2022

Good catch! You are right. We check for x.fvalue === unboundFunction to know if a symbol is bound or not, and that doesn't work anymore with .bind.

We need a way to check for it now.

davazp added 2 commits August 31, 2022 13:56
That way the error messagae will be correct regardless if we invoke it
like symbol.fvalue() or if we assign it first into a variable, losing
the `this` context.
As we're binding the function to the symbol with .bind(), we lose the
identity check for `unboundFunction`.

Instead, mark the function with a special symbol so we can detect the
symbol is unbound.
@davazp davazp force-pushed the fix-funcall-error-message branch from 7214c64 to b64a8fb Compare August 31, 2022 12:19
src/prelude.js Show resolved Hide resolved
By moving the condition to the FBOUNDP wrapper function, we have access
to the proper `ERROR` function.
@davazp
Copy link
Member Author

davazp commented Sep 1, 2022

@vlad-km thanks for the review!

@davazp davazp merged commit 22e2415 into master Sep 1, 2022
@davazp davazp deleted the fix-funcall-error-message branch September 1, 2022 09:23
@vlad-km vlad-km mentioned this pull request Sep 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants