search

move-language / move

Apache License 2.0
2.27k stars 690 forks source link

[Bug] move-analyzer --branch sui-move server crash #1082

Open thounyy opened 1 year ago

thounyy commented 1 year ago

🐛 Bug

To reproduce

Any Sui Move module, sometimes when adding:

use::

But I think it happens whenever I write a new line of code in the middle of a function or module, and it necessitates a comma but it crashes before I add it.

Stack trace/error message

symbolicator message error: RecvError

Expected Behavior

Keep highlighting the errors and warnings instead of getting stuck on an inexistant comma. Like the sui-move plug-in from movebit.

System information

Please complete the following information:

Additional context

Feel free to guide me to give more details.

awelc commented 1 year ago

But I think it happens whenever I write a new line of code in the middle of a function or module, and it necessitates a comma but it crashes before I add it.

If possible, please specify a sequence of actions that need to be taken to reproduce it. Perhaps you can write a super simple package with one empty module, open it in the IDE and describe what needs to be typed to reproduce this error?

thounyy commented 1 year ago

Created a basic repo with an example and more details. https://github.com/thounyy/moveanalyzer-crash

awelc commented 1 year ago

Created a basic repo with an example and more details. https://github.com/thounyy/moveanalyzer-crash

Thank you! Will get on it shortly!

awelc commented 1 year ago

It has taken me longer that I expected but I am on it.

I looked at the repository with repro instructions and I still don't know how to trigger it. In particular:

  1. I opened the sample project and all looks OK.
  2. I removed use std::vector;, restarted VSCode, and tried to add it again. All looks good.
  3. I don't know where the comma should be added to trigger this.

Ideally, what I would need to repro this is the following.

  1. Sample project that I can open in VSCode
  2. Sequence of editing operations that lead to a crash

I think we may have 1 but I don't know 2...

thounyy commented 11 months ago

sorry for the late answer. the issue is that it is on any project simple or complex and occurs rapidly at any time when writing any new line of code. So, for 2. it's just writing any new line of code

awelc commented 11 months ago

sorry for the late answer. the issue is that it is on any project simple or complex and occurs rapidly at any time when writing any new line of code. So, for 2. it's just writing any new line of code

I can't reproduce it. I am currently actively developing the analyzer, writing a fair amount of Move test code myself, and I did not see it crashing the way you describe. I also tried your example, typing new lines as rapidly as I can, bot at module level (new imports) and inside a function - still no crash.

I would really like to help (and make the plugin as good as I can!), and I really grateful for feedback, but without being able to reproduce it, there is not much I can do... I really need a more principled way to reproduce this problem to act on this in a timely manner...

thounyy commented 11 months ago

I understand but there's nothing more specific to do, the only thing is that MoveBit extension doesn't have the problem. I searched for more details on the crash, here is the initial messages before it loops over symbolicator message error: RecvError:

scheduled run
text document notification handled
typed AST compilation failed
thread '<unnamed>' panicked at 'called `Option::unwrap()` on a `None` value', language/move-analyzer/src/diagnostics.rs:28:61
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
symbolicator message error: RecvError

I don't know if it can help but here is the diagnostic.rs file:

// Copyright (c) The Move Contributors
// SPDX-License-Identifier: Apache-2.0

use crate::utils::get_loc;
use codespan_reporting::{diagnostic::Severity, files::SimpleFiles};
use lsp_types::{Diagnostic, DiagnosticRelatedInformation, DiagnosticSeverity, Location, Range};
use move_command_line_common::files::FileHash;
use move_ir_types::location::Loc;
use move_symbol_pool::Symbol;
use std::collections::{BTreeMap, HashMap};
use url::Url;

/// Converts diagnostics from the codespan format to the format understood by the language server.
pub fn lsp_diagnostics(
    diagnostics: &Vec<(
        codespan_reporting::diagnostic::Severity,
        &'static str,
        (Loc, String),
        Vec<(Loc, String)>,
        Vec<String>,
    )>,
    files: &SimpleFiles<Symbol, String>,
    file_id_mapping: &HashMap<FileHash, usize>,
    file_name_mapping: &BTreeMap<FileHash, Symbol>,
) -> BTreeMap<Symbol, Vec<Diagnostic>> {
    let mut lsp_diagnostics = BTreeMap::new();
    for (s, _, (loc, msg), labels, _) in diagnostics {
        let fpath = file_name_mapping.get(&loc.file_hash()).unwrap(); // line 28
        if let Some(start) = get_loc(&loc.file_hash(), loc.start(), files, file_id_mapping) {
            if let Some(end) = get_loc(&loc.file_hash(), loc.end(), files, file_id_mapping) {
                let range = Range::new(start, end);
                let related_info_opt = if labels.is_empty() {
                    None
                } else {
                    Some(
                        labels
                            .iter()
                            .filter_map(|(lloc, lmsg)| {
                                let lstart = get_loc(
                                    &lloc.file_hash(),
                                    lloc.start(),
                                    files,
                                    file_id_mapping,
                                )?;
                                let lend =
                                    get_loc(&lloc.file_hash(), lloc.end(), files, file_id_mapping)?;
                                let lpath = file_name_mapping.get(&lloc.file_hash()).unwrap();
                                let lpos = Location::new(
                                    Url::from_file_path(lpath.as_str()).unwrap(),
                                    Range::new(lstart, lend),
                                );
                                Some(DiagnosticRelatedInformation {
                                    location: lpos,
                                    message: lmsg.to_string(),
                                })
                            })
                            .collect(),
                    )
                };
                lsp_diagnostics
                    .entry(*fpath)
                    .or_insert_with(Vec::new)
                    .push(Diagnostic::new(
                        range,
                        Some(severity(*s)),
                        None,
                        None,
                        msg.to_string(),
                        related_info_opt,
                        None,
                    ));
            }
        }
    }
    lsp_diagnostics
}

/// Produces empty diagnostics in the format understood by the language server for all files that
/// the language server is aware of.
pub fn lsp_empty_diagnostics(
    file_name_mapping: &BTreeMap<FileHash, Symbol>,
) -> BTreeMap<Symbol, Vec<Diagnostic>> {
    let mut lsp_diagnostics = BTreeMap::new();
    for n in file_name_mapping.values() {
        lsp_diagnostics.insert(*n, vec![]);
    }
    lsp_diagnostics
}

/// Converts diagnostic severity level from the codespan format to the format understood by the
/// language server.
fn severity(s: Severity) -> DiagnosticSeverity {
    match s {
        Severity::Bug => DiagnosticSeverity::Error,
        Severity::Error => DiagnosticSeverity::Error,
        Severity::Warning => DiagnosticSeverity::Warning,
        Severity::Note => DiagnosticSeverity::Information,
        Severity::Help => DiagnosticSeverity::Hint,
    }
}
awelc commented 11 months ago

I don't know if it can help

Thank you! It certainly helps as it gives me an idea on what part of the implementation to look at. It's a very strange one, because:

I will look into it, but I was hoping that you could help me debug this a bit further. If you could modify the diagnostics.rs file to include some extra debugging info, rebuild move-analyzer, and include the more verbose output, it would help even more.

I am thinking something along changing line 28 from this

         let fpath = file_name_mapping.get(&loc.file_hash()).unwrap(); // line 28

into this


        let Some(fpath) = file_name_mapping.get(&loc.file_hash()) else {
            eprintln!("MAPPING FILE ERROR");
            eprintln!("LOC: {:?}", loc);
            eprintln!("MSG: {msg}");
            continue;
        };
``
thounyy commented 11 months ago

I don't know if it was expected but by modifying the line 28 like you proposed then rebuilding and installing move-analyzer from the modified repo I don't get the error anymore. I'll let you know whether it happens again or not with this config.

awelc commented 11 months ago

I don't know if it was expected but by modifying the line 28 like you proposed then rebuilding and installing move-analyzer from the modified repo I don't get the error anymore

The error (crash) will indeed not happen anymore because we hit continue before hitting the unwrap that causes the crash. You should nevertheless see the "MAPPING ERROR..." messages in the move analyzer log (where you previously saw the panic message).

thounyy commented 11 months ago

oh right, well I didn't see the messages in the output console so I guess it never goes the else branch. 

// let fpath = file_name_mapping.get(&loc.file_hash()).unwrap();
let Some(fpath) = file_name_mapping.get(&loc.file_hash()) else {
        continue;
};
eprintln!("MAPPING FILE ERROR");
eprintln!("LOC: {:?}", loc);
eprintln!("MSG: {msg}");

I moved them outside the else and it shows:

Warning: unknown field name found. Expected one of [name, version, authors, license], but found 'published-at'
compiled to typed AST
compiling to bytecode
bytecode compilation failed
MAPPING FILE ERROR
LOC: Loc { file_hash: "75e94fd81244b6eb4e67ea5d68e050997c55f3251ac75d37be03e4c73b6f7053", start: 988, end: 1054 }
MSG: Invalid address literal. The numeric value is too large. The maximum size is 16 bytes
MAPPING FILE ERROR
LOC: Loc { file_hash: "75e94fd81244b6eb4e67ea5d68e050997c55f3251ac75d37be03e4c73b6f7053", start: 1169, end: 1235 }
MSG: Invalid address literal. The numeric value is too large. The maximum size is 16 bytes
MAPPING FILE ERROR
LOC: Loc { file_hash: "75e94fd81244b6eb4e67ea5d68e050997c55f3251ac75d37be03e4c73b6f7053", start: 3366, end: 3432 }
MSG: Invalid address literal. The numeric value is too large. The maximum size is 16 bytes
MAPPING FILE ERROR
LOC: Loc { file_hash: "75e94fd81244b6eb4e67ea5d68e050997c55f3251ac75d37be03e4c73b6f7053", start: 3493, end: 3559 }
MSG: Invalid address literal. The numeric value is too large. The maximum size is 16 bytes
MAPPING FILE ERROR
LOC: Loc { file_hash: "75e94fd81244b6eb4e67ea5d68e050997c55f3251ac75d37be03e4c73b6f7053", start: 4433, end: 4499 }
MSG: Invalid address literal. The numeric value is too large. The maximum size is 16 bytes
MAPPING FILE ERROR
LOC: Loc { file_hash: "75e94fd81244b6eb4e67ea5d68e050997c55f3251ac75d37be03e4c73b6f7053", start: 4612, end: 4678 }
MSG: Invalid address literal. The numeric value is too large. The maximum size is 16 bytes
MAPPING FILE ERROR
LOC: Loc { file_hash: "75e94fd81244b6eb4e67ea5d68e050997c55f3251ac75d37be03e4c73b6f7053", start: 5542, end: 5608 }
MSG: Invalid address literal. The numeric value is too large. The maximum size is 16 bytes
MAPPING FILE ERROR
LOC: Loc { file_hash: "75e94fd81244b6eb4e67ea5d68e050997c55f3251ac75d37be03e4c73b6f7053", start: 5743, end: 5809 }
MSG: Invalid address literal. The numeric value is too large. The maximum size is 16 bytes
MAPPING FILE ERROR
LOC: Loc { file_hash: "75e94fd81244b6eb4e67ea5d68e050997c55f3251ac75d37be03e4c73b6f7053", start: 6642, end: 6708 }
MSG: Invalid address literal. The numeric value is too large. The maximum size is 16 bytes
MAPPING FILE ERROR
LOC: Loc { file_hash: "75e94fd81244b6eb4e67ea5d68e050997c55f3251ac75d37be03e4c73b6f7053", start: 6836, end: 6902 }
MSG: Invalid address literal. The numeric value is too large. The maximum size is 16 bytes
MAPPING FILE ERROR
LOC: Loc { file_hash: "81af6b215313a848bc62409d2fbf08ff86da622241c851ba993bae3cdd11f1b6", start: 368, end: 434 }
MSG: Invalid address literal. The numeric value is too large. The maximum size is 16 bytes
MAPPING FILE ERROR
LOC: Loc { file_hash: "81af6b215313a848bc62409d2fbf08ff86da622241c851ba993bae3cdd11f1b6", start: 2210, end: 2276 }
MSG: Invalid address literal. The numeric value is too large. The maximum size is 16 bytes
MAPPING FILE ERROR
LOC: Loc { file_hash: "81af6b215313a848bc62409d2fbf08ff86da622241c851ba993bae3cdd11f1b6", start: 3006, end: 3072 }
MSG: Invalid address literal. The numeric value is too large. The maximum size is 16 bytes
MAPPING FILE ERROR
LOC: Loc { file_hash: "81af6b215313a848bc62409d2fbf08ff86da622241c851ba993bae3cdd11f1b6", start: 3749, end: 3815 }
MSG: Invalid address literal. The numeric value is too large. The maximum size is 16 bytes
MAPPING FILE ERROR
LOC: Loc { file_hash: "81af6b215313a848bc62409d2fbf08ff86da622241c851ba993bae3cdd11f1b6", start: 4563, end: 4629 }
MSG: Invalid address literal. The numeric value is too large. The maximum size is 16 bytes
MAPPING FILE ERROR
LOC: Loc { file_hash: "81af6b215313a848bc62409d2fbf08ff86da622241c851ba993bae3cdd11f1b6", start: 5259, end: 5325 }
MSG: Invalid address literal. The numeric value is too large. The maximum size is 16 bytes
MAPPING FILE ERROR
LOC: Loc { file_hash: "3521f9d8d934018739962b8415c94dd20d178dbae046398ec233094ccfcae6d8", start: 370, end: 436 }
MSG: Invalid address literal. The numeric value is too large. The maximum size is 16 bytes
MAPPING FILE ERROR
LOC: Loc { file_hash: "3521f9d8d934018739962b8415c94dd20d178dbae046398ec233094ccfcae6d8", start: 706, end: 772 }
MSG: Invalid address literal. The numeric value is too large. The maximum size is 16 bytes
MAPPING FILE ERROR
LOC: Loc { file_hash: "3521f9d8d934018739962b8415c94dd20d178dbae046398ec233094ccfcae6d8", start: 1187, end: 1253 }
MSG: Invalid address literal. The numeric value is too large. The maximum size is 16 bytes
MAPPING FILE ERROR
LOC: Loc { file_hash: "ed29c699e81007b2191fc3a301f4623ef67ba778ed463774227073bdd4b61608", start: 450, end: 516 }
MSG: Invalid address literal. The numeric value is too large. The maximum size is 16 bytes
MAPPING FILE ERROR
LOC: Loc { file_hash: "ed29c699e81007b2191fc3a301f4623ef67ba778ed463774227073bdd4b61608", start: 533, end: 599 }
MSG: Invalid address literal. The numeric value is too large. The maximum size is 16 bytes
MAPPING FILE ERROR
LOC: Loc { file_hash: "ed29c699e81007b2191fc3a301f4623ef67ba778ed463774227073bdd4b61608", start: 2303, end: 2369 }
MSG: Invalid address literal. The numeric value is too large. The maximum size is 16 bytes
MAPPING FILE ERROR
LOC: Loc { file_hash: "ed29c699e81007b2191fc3a301f4623ef67ba778ed463774227073bdd4b61608", start: 2955, end: 3021 }
MSG: Invalid address literal. The numeric value is too large. The maximum size is 16 bytes
MAPPING FILE ERROR
LOC: Loc { file_hash: "ed29c699e81007b2191fc3a301f4623ef67ba778ed463774227073bdd4b61608", start: 3711, end: 3777 }
MSG: Invalid address literal. The numeric value is too large. The maximum size is 16 bytes
MAPPING FILE ERROR
LOC: Loc { file_hash: "ed29c699e81007b2191fc3a301f4623ef67ba778ed463774227073bdd4b61608", start: 4659, end: 4725 }
MSG: Invalid address literal. The numeric value is too large. The maximum size is 16 bytes
MAPPING FILE ERROR
LOC: Loc { file_hash: "ed29c699e81007b2191fc3a301f4623ef67ba778ed463774227073bdd4b61608", start: 5848, end: 5914 }
MSG: Invalid address literal. The numeric value is too large. The maximum size is 16 bytes
MAPPING FILE ERROR
LOC: Loc { file_hash: "131e0e757ab6faad31dcc3840edf4c5953a79135fb93368996336e391fd15a2d", start: 549, end: 615 }
MSG: Invalid address literal. The numeric value is too large. The maximum size is 16 bytes
MAPPING FILE ERROR
LOC: Loc { file_hash: "131e0e757ab6faad31dcc3840edf4c5953a79135fb93368996336e391fd15a2d", start: 632, end: 698 }
MSG: Invalid address literal. The numeric value is too large. The maximum size is 16 bytes
MAPPING FILE ERROR
LOC: Loc { file_hash: "131e0e757ab6faad31dcc3840edf4c5953a79135fb93368996336e391fd15a2d", start: 2622, end: 2688 }
MSG: Invalid address literal. The numeric value is too large. The maximum size is 16 bytes
MAPPING FILE ERROR
LOC: Loc { file_hash: "131e0e757ab6faad31dcc3840edf4c5953a79135fb93368996336e391fd15a2d", start: 3292, end: 3358 }
MSG: Invalid address literal. The numeric value is too large. The maximum size is 16 bytes
MAPPING FILE ERROR
LOC: Loc { file_hash: "131e0e757ab6faad31dcc3840edf4c5953a79135fb93368996336e391fd15a2d", start: 4065, end: 4131 }
MSG: Invalid address literal. The numeric value is too large. The maximum size is 16 bytes
MAPPING FILE ERROR
LOC: Loc { file_hash: "131e0e757ab6faad31dcc3840edf4c5953a79135fb93368996336e391fd15a2d", start: 5306, end: 5372 }
MSG: Invalid address literal. The numeric value is too large. The maximum size is 16 bytes
MAPPING FILE ERROR
LOC: Loc { file_hash: "5142fa98ae9eacaf764c75e7b6f5dfc135e71b4474fc7a8122cd579c2f9470eb", start: 292, end: 298 }
MSG: Unused 'use' of alias 'vector'. Consider removing it
MAPPING FILE ERROR
LOC: Loc { file_hash: "57b820ca00d12f31e1a736b276730cf240a2b4dd60395cc12eb237b1c6d6de49", start: 517, end: 527 }
MSG: Unused 'use' of alias 'tx_context'. Consider removing it
MAPPING FILE ERROR
LOC: Loc { file_hash: "57b820ca00d12f31e1a736b276730cf240a2b4dd60395cc12eb237b1c6d6de49", start: 486, end: 495 }
MSG: Unused 'use' of alias 'ActiveJwk'. Consider removing it
MAPPING FILE ERROR
LOC: Loc { file_hash: "57b820ca00d12f31e1a736b276730cf240a2b4dd60395cc12eb237b1c6d6de49", start: 244, end: 252 }
MSG: Unused 'use' of alias 'Scenario'. Consider removing it
MAPPING FILE ERROR
LOC: Loc { file_hash: "57b820ca00d12f31e1a736b276730cf240a2b4dd60395cc12eb237b1c6d6de49", start: 172, end: 178 }
MSG: Unused 'use' of alias 'String'. Consider removing it
MAPPING FILE ERROR
LOC: Loc { file_hash: "3a8d739238300aed3fc7ac79d6a7f5527c7b02b4a78b83d6bf40282e853a6d96", start: 150, end: 156 }
MSG: Unused 'use' of alias 'vector'. Consider removing it
awelc commented 11 months ago

I moved them outside the else and it shows

This unfortunately does not help any further... The motivation behind the change I proposed was to capture error-related information right before the error actually happens. The let ... else statement captures a situation when file_name_mapping.get(&loc.file_hash()) returns None which would cause an error when calling unwrap on it.

In other words, I did not really change anything in terms of the error happening or not - you should see the "MAPPING ERROR..." messages in the same situations that you saw the crash earlier.

thounyy commented 11 months ago

Indeed, it makes sense. Since I don't get the error with the let ... else statement I modified it this way:

eprintln!("MAPPING FILE ERROR");
eprintln!("LOC: {:?}", loc);
eprintln!("MSG: {msg}");
let fpath = file_name_mapping.get(&loc.file_hash()).unwrap();

here is what I get before the error:

scheduled run
text document notification handled
typed AST compilation failed
MAPPING FILE ERROR
LOC: Loc { file_hash: "b0285d7c8de6cacd5052fa47b2b9e154cb24f6fd3b2e52db5c7311b0627d42c2", start: 138, end: 141 }
MSG: Unexpected 'use'
thread '<unnamed>' panicked at language/move-analyzer/src/diagnostics.rs:31:61:
called `Option::unwrap()` on a `None` value
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
symbolicator message error: RecvError
symbolicator message error: RecvError
symbolicator message error: RecvError