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

Assignment of N makes Mathics crash #210

Closed
adamantinum opened this issue Mar 17, 2022 · 10 comments
Closed

Assignment of N makes Mathics crash #210

adamantinum opened this issue Mar 17, 2022 · 10 comments

Comments

@adamantinum
Copy link
Contributor

Assignments such as N = 1 make Mathics crash because lhs has no attribute leaves (file mathics/builtin/assignments/internals.py, line 399)

I don't know if it be better to add an hasattr, as below, or a try-catch.
Obviously it could be useful call the rejected_because_protected function.

def process_assign_n(self, lhs, rhs, evaluation, tags, upset):    
    lhs, condition = unroll_conditions(lhs)    
    lhs, rhs = unroll_patterns(lhs, rhs, evaluation)    
    defs = evaluation.definitions    
                              
    if hasattr(lhs, "leaves"):                 # <--- here
        if len(lhs.leaves) not in (1, 2):    
            evaluation.message_args("N", len(lhs.leaves), 1, 2)    
            raise AssignmentException(lhs, None)    
        if len(lhs.leaves) == 1:    
            nprec = SymbolMachinePrecision    
        else:                 
            nprec = lhs.leaves[1]        
        focus = lhs.leaves[0]    
        lhs = Expression(SymbolN, focus, nprec)    
        tags = process_tags_and_upset_dont_allow_custom(    
            tags, upset, self, lhs, focus, evaluation    
        )                     
        count = 0             
        lhs, rhs = process_rhs_conditions(lhs, rhs, condition, evaluation)      
        rule = Rule(lhs, rhs)    
        for tag in tags:      
            if rejected_because_protected(self, lhs, tag, evaluation):    
                continue      
            count += 1        
            defs.add_nvalue(tag, rule)    
        return count > 0 
@rocky
Copy link
Member

rocky commented Mar 17, 2022

From a pure programming standpoint, you don't add try unless the access where the error is being called is out of your control and breaks modularity.

It sounds like a good high-level solution is something more along the lines about checking whether the lhs is a protected function as you observe rather than a more low-level solution about whether some sort of element is compound or not.

@adamantinum
Copy link
Contributor Author

From a pure programming standpoint, you don't add try unless the access where the error is being called is out of your control and breaks modularity.

It sounds like a good high-level solution is something more along the lines about checking whether the lhs is a protected function as you observe rather than a more low-level solution about whether some sort of element is compound or not.

Since the error is the missing leaves, could be possibie to inizialize an empty leaves, so process_assign_n() function exercutes without error?

@TiagoCavalcante
Copy link
Contributor

TiagoCavalcante commented Mar 17, 2022

@adamantinum I'm not sure if that makes sense, if I understand correctly this is happening with only special symbols, and they really shouldn't be re-assigned because the core of Mathics uses them, for example, any type of graphics would not work without N, it is kinda like if Python allowed def = 1.

I believe this should be something like the following:

if not lhs.hasattr("leaves"):
  return evaluation.warning("You really shouldn't do this")

(warning: pseudo-code above)

@adamantinum
Copy link
Contributor Author

@adamantinum I'm not sure if that makes sense, if I understand correctly this is happening with only special symbols, and they really shouldn't be re-assigned because the core of Mathics uses them, for example, any type of graphics would not work without N, it is kinda like if Python allowed def = 1.

As far as I tried it happens only with N, if I run e.g. If=1 I get correctly "Set::wrsym: Symbol If is Protected."
To get the same result with N I had to comment all lines which call leaves attribute. I don't know the difference in implementation, but initializing leaves would do the job (despite it be an ugly workaround)

@TiagoCavalcante
Copy link
Contributor

@adamantinum I'm not sure if that makes sense, if I understand correctly this is happening with only special symbols, and they really shouldn't be re-assigned because the core of Mathics uses them, for example, any type of graphics would not work without N, it is kinda like if Python allowed def = 1.

As far as I tried it happens only with N, if I run e.g. If=1 I get correctly "Set::wrsym: Symbol If is Protected." To get the same result with N I had to comment all lines which call leaves attribute. I don't know the difference in implementation, but initializing leaves would do the job (despite it be an ugly workaround)

@adamantinum with "do the job" it lets the user assign or show the warning? Anyway, if you could make a PR to we test, that'd be great.

@rocky @mmatera this is showing that something is really wrong, N should be like any symbol. I don't know even the place to look for this, do you have an idea?

@adamantinum
Copy link
Contributor Author

@adamantinum I'm not sure if that makes sense, if I understand correctly this is happening with only special symbols, and they really shouldn't be re-assigned because the core of Mathics uses them, for example, any type of graphics would not work without N, it is kinda like if Python allowed def = 1.

As far as I tried it happens only with N, if I run e.g. If=1 I get correctly "Set::wrsym: Symbol If is Protected." To get the same result with N I had to comment all lines which call leaves attribute. I don't know the difference in implementation, but initializing leaves would do the job (despite it be an ugly workaround)

@adamantinum with "do the job" it lets the user assign or show the warning? Anyway, if you could make a PR to we test, that'd be great.

@rocky @mmatera this is showing that something is really wrong, N should be like any symbol. I don't know even the place to look for this, do you have an idea?

It means it returns the warning N is a protected symbol, as it does for any other Mathics symbol.
I'll try to make a PR, after I'll have understood how leaves attribute works and is defined

@mmatera
Copy link
Contributor

mmatera commented Mar 17, 2022

I am working on it. I hope to have it later today.

mmatera added a commit that referenced this issue Mar 17, 2022
@mmatera mmatera mentioned this issue Mar 17, 2022
@adamantinum
Copy link
Contributor Author

That's perfect, with the PR it works as it should do. Is there a reason why N behaves differently from other Symbols?

@TiagoCavalcante regarding the place where looking for things like that, isn't there a manual or a scheme of how mathics code is organized?

@TiagoCavalcante
Copy link
Contributor

TiagoCavalcante commented Mar 17, 2022

@adamantinum yes, there is buy it isn't that complete, anyway that isn't necessary as mmatera fixed it.

@adamantinum
Copy link
Contributor Author

@adamantinum yes, there is buy it isn't that complete, anyway that isn't necessary as mmatera fixed it.

Yes, I've seen the PR.
I'm interested in the manual mostly regarding future contributions.

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

No branches or pull requests

4 participants