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

stale diagnostic after panic #18732

Open
rbtcollins opened this issue Dec 21, 2024 · 6 comments
Open

stale diagnostic after panic #18732

rbtcollins opened this issue Dec 21, 2024 · 6 comments
Labels
C-bug Category: bug

Comments

@rbtcollins
Copy link

Very weird - basic code reporting a Syntax error, but it runs fine via Cargo run :)

rust-analyzer version: rust-analyzer version: 0.3.2220-standalone (27e824f 2024-12-15) [c:\Users\robertc.vscode\extensions\rust-lang.rust-analyzer-0.3.2220-win32-x64\server\rust-analyzer.exe]

rustc version: rustc 1.83.0 (90b35a623 2024-11-26)

editor or extension: VSCode Version 0.3.2220

relevant settings: (eg. client settings, or environment variables like CARGO, RUSTC, RUSTUP_HOME or CARGO_HOME)

rustup show
$ rustup show
Default host: x86_64-pc-windows-msvc
rustup home:  C:\Users\robertc\.rustup

...

installed targets for active toolchain
--------------------------------------

x86_64-pc-windows-msvc
x86_64-unknown-linux-gnu

active toolchain
----------------

stable-x86_64-pc-windows-msvc (default)
rustc 1.83.0 (90b35a623 2024-11-26)

code snippet to reproduce:

use std::collections::HashMap;

use crate::eval::Value;

pub struct Environment {
    values: HashMap<String, Value>,
}

impl Environment {
    pub fn new() -> Self {
        Self {
            values: HashMap::new(),
        }
    }

    pub fn define(&mut self, name: String, value: Value) {
        self.values.insert(name, value);
    }

    pub fn get(&self, name: &str) -> Result<Value, Box<dyn std::error::Error>> {
        self.values
            .get(name)
            .cloned()
            .ok_or_else(|| format!("Undefined variable {name}").into())
    }
}

Reports these problems:

[{
	"resource": "/c:/Users/robertc/Documents/src/codecrafters-interpreter-rust/src/environment.rs",
	"owner": "_generated_diagnostic_collection_name_#1",
	"code": {
		"value": "syntax-error",
		"target": {
			"$mid": 1,
			"path": "/stable/reference/",
			"scheme": "https",
			"authority": "doc.rust-lang.org"
		}
	},
	"severity": 8,
	"message": "Syntax Error: expected a block",
	"source": "rust-analyzer",
	"startLineNumber": 17,
	"startColumn": 16,
	"endLineNumber": 17,
	"endColumn": 16
}]

and

[{
	"resource": "/c:/Users/robertc/Documents/src/codecrafters-interpreter-rust/src/environment.rs",
	"owner": "_generated_diagnostic_collection_name_#1",
	"code": {
		"value": "E0308",
		"target": {
			"$mid": 1,
			"path": "/stable/error_codes/E0308.html",
			"scheme": "https",
			"authority": "doc.rust-lang.org"
		}
	},
	"severity": 8,
	"message": "expected String, found &str",
	"source": "rust-analyzer",
	"startLineNumber": 18,
	"startColumn": 28,
	"endLineNumber": 18,
	"endColumn": 32
}]

Image

@rbtcollins rbtcollins added the C-bug Category: bug label Dec 21, 2024
@ChayimFriedman2
Copy link
Contributor

When some obviously-not-syntax-error reports a SyntaxError, that usually happens in my experience after the diagnostics request has panicked. So, did you saw a popup about the server panicking before?

@rbtcollins
Copy link
Author

I don't recall one, but the subtle corner popups in vscode rarely attract my eyeballs.

I can see this in the RA LSP output:

Panic context:
> 
version: 0.3.2220-standalone (27e824fad4 2024-12-15)
request: textDocument/diagnostic DocumentDiagnosticParams {
    text_document: TextDocumentIdentifier {
        uri: Url {
            scheme: "file",
            cannot_be_a_base: false,
            username: "",
            password: None,
            host: None,
            port: None,
            path: "/c%3A/Users/robertc/Documents/src/codecrafters-interpreter-rust/src/environment.rs",
            query: None,
            fragment: None,
        },
    },
    identifier: None,
    previous_result_id: None,
    work_done_progress_params: WorkDoneProgressParams {
        work_done_token: None,
    },
    partial_result_params: PartialResultParams {
        partial_result_token: None,
    },
}

thread 'Worker' panicked at crates\syntax\src\ast\make.rs:117:5:
Failed to make ast node `syntax::ast::generated::nodes::Name` from text mod self;
stack backtrace:
   0: std::panicking::begin_panic_handler
             at /rustc/90b35a6239c3d8bdabc530a6a0816f7ff89a0aaf\library/std\src\panicking.rs:665
   1: core::panicking::panic_fmt
             at /rustc/90b35a6239c3d8bdabc530a6a0816f7ff89a0aaf\library/core\src\panicking.rs:74
   2: syntax::ast::make::name
   3: ide_diagnostics::handlers::unresolved_field::unresolved_field
   4: ide_diagnostics::semantic_diagnostics
   5: ide_diagnostics::full_diagnostics
   6: ra_salsa::Cancelled::catch
   7: ide::Analysis::full_diagnostics
   8: rust_analyzer::handlers::request::handle_document_diagnostics
   9: core::ops::function::FnOnce::call_once{{vtable.shim}}
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
[Error - 3:40:10 PM] Request textDocument/diagnostic failed.
  Message: request handler panicked: Failed to make ast node `syntax::ast::generated::nodes::Name` from text mod self;
  Code: -32603 
[Error - 3:40:10 PM] Document pull failed for text document file:///c%3A/Users/robertc/Documents/src/codecrafters-interpreter-rust/src/environment.rs
  Message: request handler panicked: Failed to make ast node `syntax::ast::generated::nodes::Name` from text mod self;
  Code: -32603 

But I don't know if its connected.

I have restarted the LSP but the problem remained.

@ChayimFriedman2
Copy link
Contributor

Yes this means there was a panic in diagnostics. Restarting rust-analyzer should work unless it panics again.

@Veykril
Copy link
Member

Veykril commented Dec 21, 2024

Its definitely a bug that a panic there can cause us to leak (never clear) diagnostics

@ChayimFriedman2
Copy link
Contributor

I think this is because the diagnostics request is not wrapped in catch_unwind().

@Veykril
Copy link
Member

Veykril commented Dec 22, 2024

Ah right, I was looking at the push model style diagnostic handling, not the pull one that is actually being used.
It is implicitly wrapped by the thread pool handling so the issue lies with us returning an error it seems.
That seems a bit odd then, I'd expect vscode to ask for diagnostics again when the doc changes, causing it clear them afterwards when we give it a proper result back.

So this smells like a VSCode issue to me (though that is not confirmed). We do set an id for our pull diagnostics now though, so maybe that makes a difference (next release)

@Veykril Veykril changed the title Syntax Error: expected a block stale diagnostic after panic Dec 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: bug
Projects
None yet
Development

No branches or pull requests

3 participants