warn when preparser redefines Python builtin function

[dandrake]

At https://groups.google.com/forum/?fromgroups=#!topic/sage-devel/EQzNB7vkN3o , a user tries this:

sage: t=(1,2,3)
sage: type(t)
tuple
sage: len(t)
3
sage: len(t)=4
sage: t
t
sage: type(t)
sage.symbolic.expression.Expression

This strange behavior is because of the preparser:

sage: preparse('len(t) = 4')
'__tmp__=var("t"); len = symbolic_expression(Integer(4)).function(t)'

I propose that when the preparser is about to redefine a Python built-in function, we issue a warning. This will allow users to clobber the builtins, but also tell them they are possibly doing something unwise.

CC: @jasongrout

Component: user interface

Keywords: symbolic preparser builtin built-in

Author: Dan Drake

Issue created by migration from https://trac.sagemath.org/ticket/14231

[dandrake]

comment:1

Attached is a first draft of a patch. Two issues: (1) I don't know how to properly raise the warning; right now it just prints it, but perhaps we have a more sophisticated mechanism that should be used? (2) It seems like all lines from the IPython shell get preparsed twice, so the warning is printed twice. I haven't dug into the preparser enough to understand this. But the attached patch is at least a sample of what I propose.

[dandrake]

Author: Dan Drake

[nbruin]

comment:3

How about the list:

__builtin__.__dict__.keys()

If the problem is detected you could insert a warning printing message into the preparsing result. Then the warning printing gets executed together with the symbol binding.

The real problem here is that previously mistyping len(t)==4 as len(t)=4 resulted in invalid syntax, whereas now it results in an assignment. The same surprise happens when someone types

t = 0
sin(t) = 0 # mistyping of sin(t) == 0

Do you want to guard against that as well? Then quickly the whole f(t)=... notation should lead to a warning (which I wouldn't mind because I think it's one of the places where the preprocessor is too intrusive)

In fact, always printing:

sage: f(u,v)=u+v
Defining symbolic function f and symbolic variables u,v

might not be so bad. We're defining new syntax. We can define new semantics for that syntax, right?
We could have that result by ensuring that

sage: preparse('f(u,v)=u+v')
'__tmp__=var("u,v"); f = symbolic_expression(u+v).function(u,v); print "Defining f and u,v"'

or whatever (terse) message would be reasonable.

[dandrake]

comment:4

Replying to @nbruin:

How about the list:

__builtin__.__dict__.keys()

That's much smarter than my cut-and-paste job. Thanks!

In fact, always printing:

sage: f(u,v)=u+v
Defining symbolic function f and symbolic variables u,v

might not be so bad. We're defining new syntax. We can define new semantics for that syntax, right?

I like that. It alleviates the clobbering problem (both for "len" and "sin"), and makes it clear to the user what we are doing. I taught the MAA PREP workshop on Sage last summer, and defining symbolic functions and variables was a sticky point. This would at least give the user a warning and something to report ("Sage said it defined a symbolic function...").

However, fixing all the doctests will turn this into a bit of a patchbomb. With the above, there's about 35 files and about 300 doctests to fix. And such a change in behavior will require appropriate bikeshedding arguments on sage-devel. :)

I'll post my patch (without doctest fixes) and solicit comments.

[nbruin]

comment:5

Or, slightly modified:

sage: preparse('f(u,v)=u+v')
'__tmp__=var("u,v"); f = symbolic_expression(u+v).function(u,v); "Defining f and u,v"'

would provide another flavour. Here, essentially the warning is the "value" of the assignment, so it only gets printed if values of statements do (i.e., in interactive environments).

If you want to get rid of the quotes in printing:

class NonQuoteRepr(str):
    def __repr__(a):
        return a

and return

NonQuoteRepr("Defining f and u,v")

instead. Perhaps we already have a class for representing messages that does this.

[dandrake]

Attachment: trac_14231.patch.gz

[dandrake]

comment:6

Replying to @nbruin:

If you want to get rid of the quotes in printing:

class NonQuoteRepr(str):
    def __repr__(a):
        return a

and return

NonQuoteRepr("Defining f and u,v")

instead.

I tried that but it doesn't seem to work -- the result is printed in doctest mode anyway. The current version of the patch does this; apply it and doctest the calculus directory. Any ideas? I think using DOCTEST_MODE is probably the best way here.

[jhpalmieri]

comment:7

Regarding the message: I think that this behavior needs to be documented. If the main reason for using DOCTEST_MODE or similar is to avoid a patchbomb, it might be better to bite the bullet and patch all of the files. I think that DOCTEST_MODE is more appropriately used when you want to disable displaying a plot or opening up a web page: something which would be disruptive when running doctests — it shouldn't be used as a replacement for writing accurate doctests.

But I'm willing to be convinced otherwise.

[dandrake]

comment:8

Replying to @jhpalmieri:

Regarding the message: I think that this behavior needs to be documented. If the main reason for using DOCTEST_MODE or similar is to avoid a patchbomb, it might be better to bite the bullet and patch all of the files.

I think I agree. It seems like using DOCTEST_MODE here would just be a way to avoid a big patch. We should use that when it truly makes sense to suppress output and I don't see that here. If we're going to change basic behavior, it seems reasonable that the new behavior be reflected in doctests.

I'll fix the doctests, but it'll be a large, bitrot-prone patch (or set of patches), so before proceeding I'd like some more feedback on whether people really want this.

[kcrisman]

comment:9

Although I agree that this can be a problem, I wonder if this is too far from "standard" Python. Maybe it's better to teach people that you can clobber things, like here. I guess I'm agnostic on this proposal as of yet, though I suppose var('y') does something sort of similar - maybe it should have a longer message, too?