Skip to content

Comments

[red-knot] Fix bug where union of two iterable types was not recognised as iterable#13992

Merged
AlexWaygood merged 3 commits intomainfrom
alex/iter-bug
Oct 30, 2024
Merged

[red-knot] Fix bug where union of two iterable types was not recognised as iterable#13992
AlexWaygood merged 3 commits intomainfrom
alex/iter-bug

Conversation

@AlexWaygood
Copy link
Member

@AlexWaygood AlexWaygood commented Oct 30, 2024

Fixes #13990. My favourite kind of bugfix: more accuracy for less code!

@AlexWaygood AlexWaygood force-pushed the alex/iter-bug branch 2 times, most recently from cf8a4c4 to 5ce65e3 Compare October 30, 2024 11:43
@AlexWaygood AlexWaygood enabled auto-merge (squash) October 30, 2024 11:51
@AlexWaygood AlexWaygood merged commit 42c7069 into main Oct 30, 2024
@AlexWaygood AlexWaygood deleted the alex/iter-bug branch October 30, 2024 11:54
@github-actions
Copy link
Contributor

github-actions bot commented Oct 30, 2024

ruff-ecosystem results

Linter (stable)

ℹ️ ecosystem check encountered linter errors. (no lint changes; 1 project error)

pandas-dev/pandas (+0 -0 violations, +0 -0 fixes) 







pypa/setuptools (error)



ruff failed
  Cause: Failed to parse /home/runner/work/ruff/ruff/checkouts/pypa:setuptools/ruff.toml
  Cause: TOML parse error at line 1, column 11
  |
1 | include = "pyproject.toml"
  |           ^^^^^^^^^^^^^^^^
invalid type: string "pyproject.toml", expected a sequence







Linter (preview)


ℹ️ ecosystem check encountered linter errors. (no lint changes; 1 project error)


pypa/setuptools (error)



ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview






ruff failed
  Cause: Failed to parse /home/runner/work/ruff/ruff/checkouts/pypa:setuptools/ruff.toml
  Cause: TOML parse error at line 1, column 11
  |
1 | include = "pyproject.toml"
  |           ^^^^^^^^^^^^^^^^
invalid type: string "pyproject.toml", expected a sequence







      		
    

  





        


            

            
            

              
            

        

      


      

            
  
  

    
  
    
    

  

  

  


    












      

  
            

    

      
    

    


      


          @AlexWaygood
AlexWaygood




          added
  the 

  ty

  Multi-file analysis & type inference
 label


      Oct 30, 2024

    

  








      

  
        

  

        

  MichaReiser
  

  
            

              
                    MichaReiser
  

              

              reviewed


              
                  
                    Oct 30, 2024
                  
              
              
            


            



                
    View reviewed changes
  


            





          

  
  
  
              

                      

  

    

      

          
    
    

  


        
            
  
      
              Copy link

  

        
      

    


    

        

  
  Member


        

    

  


  

    

      
          @MichaReiser
      

      
            MichaReiser
  

      

      

      left a comment




      
    


  



                  

                      

  

  
 

  
    

    
Choose a reason for hiding this comment


    

      The reason will be displayed to describe this comment to others. Learn more.
    


    

      
      



  



  
    

      
It would be nice to also have a test where one type is iterable and the other is not. E.g. iterator over str | int.


This should work as expected today but I think the fix you pushed now would have regressed the diagnostic precision when our errors distinguish between a non-iterable and an iterator missing the __next__ method.

      
    

  
  




                    

                  
  
  

    
  
    
    

  

  

  



                      

                    
                  








      

  
  
  




  








      

  
      



  

  @AlexWaygood






  



    

      

  

    

      

          
    
    

  


        
            
  
      
              Copy link

  

        
      

    


    

        

  
  Member


        

  Author


    

  


  

    

      
          
      

      
            AlexWaygood
  

      

      

      commented


        Oct 30, 2024


      
    


  





      


        



  
    

      
          


This should work as expected today




in fact, it does not... great catch!


      		
    

  





        


            

            
            

              
            

        

      


      

            
  
  

    
  
    
    

  

  

  


    












      

  
      



  

  @carljm






  



    

      

  

    

      

          
    
    

  


        
            
  
      
              Copy link

  

        
      

    


    

        

  
    Contributor


        

    

  


  

    

      
          
      

      
            carljm
  

      

      

      commented


        Oct 30, 2024


      
    


  





      


        



  
    

      
          


in fact, it does not




Should we capture this in an issue?


      		
    

  





        


            

            
            

              
            

        

      


      

            
  
  

    
  
    
    

  

  

  


    












      

  
      



  

  @AlexWaygood






  



    

      

  

    

      

          
    
    

  


        
            
  
      
              Copy link

  

        
      

    


    

        

  
  Member


        

  Author


    

  


  

    

      
          
      

      
            AlexWaygood
  

      

      

      commented


        Oct 30, 2024


      
    


  





      


        



  
    

      
          




in fact, it does not




Should we capture this in an issue?




I am working on a fix. But if I do not succeed in putting up the fix, I will write up an issue.


      		
    

  





        


            

            
            

              
            

        

      


      

            
  
  

    
  
    
    

  

  

  


    












      

  
          

  

    
  


  

      


    @AlexWaygood
AlexWaygood



    mentioned this pull request
    
      Oct 31, 2024
    





  


  

      

  
  Closed

  







  









      

  
      



  

  @AlexWaygood






  



    

      

  

    

      

          
    
    

  


        
            
  
      
              Copy link

  

        
      

    


    

        

  
  Member


        

  Author


    

  


  

    

      
          
      

      
            AlexWaygood
  

      

      

      commented


        Oct 31, 2024


      
    


  





      


        



  
    

      
          






in fact, it does not




Should we capture this in an issue?




I am working on a fix. But if I do not succeed in putting up the fix, I will write up an issue.




I did not succeed in putting up the fix! Here is the issue instead: #14012


      		
    

  





        


            

            
            

              
            

        

      


      

            
  
  

    
  
    
    

  

  

  


    
















  
  

    

      

  










  








  
      

  
    Sign up for free
    to join this conversation on GitHub.
    Already have an account?
    Sign in to comment


  










      

  

    
    

    

  

    Reviewers
  


    


      
  

    


  
      @MichaReiser
  
    MichaReiser



    
      
        
      
    
    MichaReiser left review comments

  


      

          


  
      @carljm
  
    carljm


                Awaiting requested review from carljm


        
      


      






    
    

    

  


      
  

    Assignees
  



        


    No one assigned







      


  


  

    Labels
  



    

      

    ty

  Multi-file analysis & type inference








      

  

    

        

    Projects
  


        




    None yet




  



      


  

    
  

    Milestone
  


      No milestone





    
      
          


  

    

      
        

          
  

    Development
  



            


Successfully merging this pull request may close these issues.



  
          
      [red-knot] Union incorrectly reported as not iterable



  
  



      
    

  




      

    

  
      

  

    

      3 participants
    

    

        
          @AlexWaygood 
        
          @carljm 
        
          @MichaReiser