- 
          
- 
                Notifications
    You must be signed in to change notification settings 
- Fork 3k
Defer all types whos metaclass is not ready #13579
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
Conversation
5f310f2    to
    8fbd00a      
    Compare
  
    | I got it right to pass my test, but now we have different problems: Big 'thank you' goes to @ilevkivskyi for pushing me in the right direction. | 
sympy metaclass problem
      
        
              This comment has been minimized.
        
        
      
    
  This comment has been minimized.
      
        
              This comment has been minimized.
        
        
      
    
  This comment has been minimized.
      
        
              This comment has been minimized.
        
        
      
    
  This comment has been minimized.
      
        
              This comment has been minimized.
        
        
      
    
  This comment has been minimized.
      
        
              This comment has been minimized.
        
        
      
    
  This comment has been minimized.
      
        
              This comment has been minimized.
        
        
      
    
  This comment has been minimized.
| @ilevkivskyi maybe you have any ideas where we can safely call  
 Looks like I am stuck: it still fails for some cases 😒 | 
      
        
              This comment has been minimized.
        
        
      
    
  This comment has been minimized.
| As I mentioned yesterday on discord, you will likely need to special-case  | 
      
        
              This comment has been minimized.
        
        
      
    
  This comment has been minimized.
    
      
        1 similar comment
      
    
  
    
      
        
              This comment has been minimized.
        
        
      
    
  This comment has been minimized.
      
        
              This comment has been minimized.
        
        
      
    
  This comment has been minimized.
    
      
        1 similar comment
      
    
  
    
      
        
              This comment has been minimized.
        
        
      
    
  This comment has been minimized.
| Thank you for your guidance! Without it I would spend so much more time. | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks good, I have couple more suggestions/ideas.
        
          
                mypy/semanal.py
              
                Outdated
          
        
      | # We do it in the very last order, because of | ||
| # `builtins.dict <-> typing.Mapping <-> abc.ABCMeta` | ||
| # cyclic imports. | ||
| if file_node.fullname not in ("typing", "abc", "builtins"): | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would instead re-use core_modules (defined in semanal_main.py) here and in other place. Also you may need to use a flag for this method to avoid an import cycle with the latter (like skip_implicit_attrs).
        
          
                mypy/semanal.py
              
                Outdated
          
        
      | self.add_implicit_module_attrs(file_node) | ||
| # We do it in the very last order, because of | ||
| # `builtins.dict <-> typing.Mapping <-> abc.ABCMeta` | ||
| # cyclic imports. | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is not the cyclic import itself, but that the method assumes that e.g. dict (for __annotations__) would be never marked incomplete (i.e. it may be not defined yet, but as soon as it is, it will be TypeInfo, rather than PlaceholderNode). This makes me think maybe a cleaner solution (if it works), would be just to replace assert with an if (and simply continue if it is a placeholder, like we do if the symbol is None). Can you try this?
Btw curiously this seem to only affect tests for now, it looks in real typeshed currently dict is not an instance of ABCMeta.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you try this?
Done, looks like it works! Let's wait to see what tests will show us :)
| mypy_primer fails with: I am sure it is not related, I will try to retrigger it a bit later. | 
| 
 We have the same problem on all typeshed PRs at the moment: | 
      
        
              This comment has been minimized.
        
        
      
    
  This comment has been minimized.
    
      
        1 similar comment
      
    
  
    | According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉 | 
| Thanks everyone! Next is #13565 | 
Finally! I've spent more than 8 hours on this test.
It illustrates a problem with
sympyfrom #13565