- 
                Notifications
    
You must be signed in to change notification settings  - Fork 13.9k
 
          Use translatable diagnostics in rustc_const_eval
          #111677
        
          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
  
    Use translatable diagnostics in rustc_const_eval
  
  #111677
              Conversation
| 
           r? @oli-obk (rustbot has picked a reviewer for you, use r? to override)  | 
    
| 
           
 cc @davidtwco, @compiler-errors, @JohnTitor, @TaKO8Ki Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt Some changes occurred to the CTFE / Miri engine cc @rust-lang/miri This PR changes MIR cc @oli-obk, @RalfJung, @JakobDegen, @davidtwco, @celinval, @vakaras Some changes occurred to the CTFE / Miri engine cc @rust-lang/miri  | 
    
      
        
              This comment has been minimized.
        
        
      
    
  This comment has been minimized.
dc3d10b    to
    6fbb7b3      
    Compare
  
    
      
        
              This comment has been minimized.
        
        
      
    
  This comment has been minimized.
| 
           We have plans to change the error rendering for const-eval and Miri errors quite a bit, see rust-lang/miri#2200. I have no idea how that interacts with these translation changes, does it make them harder or easier? I expect harder because now there is another huge new subsystem one has to learn to even being doing such a change.  | 
    
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.
If this has a dedicated message now, why should it still use the Custom variant?
Or conversely, if Custom variants are now 'typed' like this, then why do we need the other variants at all?
Also, for Miri we can still use arbitrary strings here, right?
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.
You are right that this doesn't need to be Custom, but I think this should be done as followup work since this PR is already enormous.
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'm fine with leaving FIXMEs, I was just trying to understand why the code looks the way it does. If anything I feel it'd make sense to move more messages to Custom than the other way around, but I might be missing some parts of the trade-off here.
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.
Are all these types here auto-generated or do they need to be manually synced with the .flt file (in terms of field/variable names etc)?
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.
Manually synced.
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.
Uff, that's suboptimal, and so easy to get wrong. I thought "there is no variable named foo" is an error you see at runtime in Python bot not Rust ;) . Are there plans to improve this? Currently it looks like translations introduce quite a bit of boilerplate and make the code significantly more fragile. I'm very happy the core interpreter rarely changes these days, or else I would be worried about its future development being impacted.
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.
Yeah it's definitely increasing the work required to add a variant to InterpError.
I think it would be nice if there is a DSL that generated both fluent resources and the Rust types that derived Diagnostic and Subdiagnostic, but I'm not sure if that's the plan.
cc @rust-lang/wg-diagnostics any thoughts on this?
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 have a PR to validate variables used open (#111269) but it unfortunately doesn't work with subdiagnostics
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.
fluent is already doing some form of code generation (right? I saw some use statements that look like this), so I was hoping a fluent line like
middle_uninit_unsized_local =
    unsized local is used while uninitialized {$variable}
could auto-generate a corresponding Rust struct with a field for the variable. Then we don't need a separate pass to check things, we get the check just directly as part of compilation.
I assume there are good reasons for why fluent works the way it does, but it sure does not feel very "rust-y" to me.
          
 I think I could do it in this PR by adding an enum that prints brief "title"s for errors. This PR also makes it impossible to use   | 
    
        
          
                tests/ui/consts/const-eval/heap/alloc_intrinsic_nontransient_fail.stderr
              
                Outdated
          
            Show resolved
            Hide resolved
        
      
          
 Probably better to separate functional changes from major refactors like this. However Miri currently relies quite a bit on the ability to put InterpError into the title...  | 
    
| 
           ☔ The latest upstream changes (presumably #111680) made this pull request unmergeable. Please resolve the merge conflicts.  | 
    
| 
           The Miri subtree was changed cc @rust-lang/miri 
  | 
    
4c49144    to
    bc2f393      
    Compare
  
    
      
        
              This comment has been minimized.
        
        
      
    
  This comment has been minimized.
| 
           Some changes occurred in src/tools/clippy cc @rust-lang/clippy  | 
    
      
        
              This comment has been minimized.
        
        
      
    
  This comment has been minimized.
      
        
              This comment has been minimized.
        
        
      
    
  This comment has been minimized.
5d2f4a1    to
    b569845      
    Compare
  
    | 
           fixed ci error @bors r=oli-obk,RalfJung  | 
    
| 
           📌 Commit b569845b920793b9797c2a704a24a11569e18d2a has been approved by  It is now in the queue for this repository.  | 
    
| 
           🔒 Merge conflict This pull request and the master branch diverged in a way that cannot be automatically merged. Please rebase on top of the latest master branch, and let the reviewer approve again. How do I rebase?Assuming  
 You may also read Git Rebasing to Resolve Conflicts by Drew Blessing for a short tutorial. Please avoid the "Resolve conflicts" button on GitHub. It uses  Sometimes step 4 will complete without asking for resolution. This is usually due to difference between how  Error message | 
    
b569845    to
    f6c2bc5      
    Compare
  
    | 
           @bors p=1 (bitrotty)  | 
    
| 
           rebased @bors r=oli-obk,RalfJung  | 
    
| 
           ☀️ Test successful - checks-actions  | 
    
| 
           Finished benchmarking commit (33c3d10): comparison URL. Overall result: ❌ regressions - no action needed@rustbot label: -perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment. 
 Max RSS (memory usage)This benchmark run did not return any relevant results for this metric. CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 644.915s -> 644.751s (-0.03%)  | 
    
This PR:
no_spanparameter tonote/helpattributes when usingSubdiagnosticto allow adding notes/helps without using a span