Prepare lookup of flags using old, deprecated names

Still blocked by upstream Bazel-side change before it can actually light up.

Author: vogelsgesang

README.md

@@ -63,22 +63,23 @@ Backlog items:
 * Diagnose
   * ✔ unknown flags
   * ✔ allow custom setting flags (`--//my/package:setting` and `--no//my/package:setting`)
-  * repeated flags
-  * abbreviated flag names; prefer non-abbreviated flags
+  * ✔ abbreviated flag names; prefer non-abbreviated flags
+  * when using an old, deprecated name (blocked on [up-stream Bazel change](https://github.com/bazelbuild/bazel/pull/25169))
   * ✔ diagnose deprecated flags
   * ✔ diagnose missing `import`ed files
   * ✔ configs on `startup`, `import`, `try-import`
   * ✔ empty config name
   * ✔ config name which doesn't match `[a-z_\-]+` (or similar)
+  * repeated flags
   * offer fix-it:
-    * to remove repeated flags
     * to replace abbreviated flags by non-abbreviated flags
     * to remove deprecated no-op flags
-    * to fix config-name-related issues
-* ✔ Hover
+    * to remove repeated flags
+* Hover
   * ✔ Show documentation of flags on hover
   * ✔ Correctly escape `<>` in Markdown (e.g. problematic in the documentation for `--config`)
   * Link to flag documentation in hovers
+  * Expose default value, value description (blocked on [up-stream Bazel change](https://github.com/bazelbuild/bazel/pull/25169))
   * ✔ Show documentation for commands on hover
 * Autocomplete
   * ✔ auto complete command names
@@ -109,8 +110,6 @@ Backlog items:
   * write documentation, including explanation of different styles
 * ✔ link file names for `import` & `try-import`
 * Rename functionality for config names
-* Bazel-side changes:
-  * expose default value, value description and old names and deprecation messages
 * Go to Reference:
-  * other usages of config name
-  * find other usages of same flag
+  * Other usages of config name
+  * Find other usages of same flag

src/bazel_flags.rs

@@ -47,6 +47,13 @@ pub struct BazelFlags {
     pub flags_by_abbreviation: HashMap<String, usize>,
 }
 
+#[derive(Clone, Debug, PartialEq, Eq)]
+pub enum FlagLookupType {
+    Normal,
+    Abbreviation,
+    OldName,
+}
+
 impl BazelFlags {
     pub fn from_flags(flags: Vec<FlagInfo>, bazel_version: Option<&str>) -> BazelFlags {
         // Index the flags from the protobuf description
@@ -59,11 +66,14 @@ impl BazelFlags {
             {
                 continue;
             }
+            flags_by_name.insert(f.name.clone(), i);
+            if let Some(old_name) = &f.old_name {
+                flags_by_name.insert(old_name.clone(), i);
+            }
             for c in &f.commands {
                 let list = flags_by_commands.entry(c.clone()).or_default();
                 list.push(i);
             }
-            flags_by_name.insert(f.name.clone(), i);
             if let Some(abbreviation) = &f.abbreviation {
                 flags_by_abbreviation.insert(abbreviation.clone(), i);
             }
@@ -98,7 +108,7 @@ impl BazelFlags {
         }
     }
 
-    pub fn get_by_invocation(&self, s: &str) -> Option<&FlagInfo> {
+    pub fn get_by_invocation(&self, s: &str) -> Option<(FlagLookupType, &FlagInfo)> {
         let stripped = s.strip_suffix('=').unwrap_or(s);
         // Long names
         if let Some(long_name) = stripped.strip_prefix("--") {
@@ -107,10 +117,17 @@ impl BazelFlags {
             }
             // Strip the `no` prefix, if any
             let stripped_no = long_name.strip_prefix("no").unwrap_or(long_name);
-            return self
-                .flags_by_name
-                .get(stripped_no)
-                .map(|i| self.flags.get(*i).unwrap());
+            return self.flags_by_name.get(stripped_no).map(|i| {
+                let flag = self.flags.get(*i).unwrap();
+                let old_name =
+                    flag.old_name.is_some() && flag.old_name.as_ref().unwrap() == stripped_no;
+                let lookup_mode = if old_name {
+                    FlagLookupType::OldName
+                } else {
+                    FlagLookupType::Normal
+                };
+                (lookup_mode, flag)
+            });
         }
         // Short names
         if let Some(abbreviation) = stripped.strip_prefix('-') {
@@ -120,7 +137,7 @@ impl BazelFlags {
             return self
                 .flags_by_abbreviation
                 .get(abbreviation)
-                .map(|i| self.flags.get(*i).unwrap());
+                .map(|i| (FlagLookupType::Abbreviation, self.flags.get(*i).unwrap()));
         }
         None
     }
@@ -210,7 +227,7 @@ pub fn combine_key_value_flags(lines: &mut [crate::parser::Line], bazel_flags: &
             new_flags.push(
                 || -> Option<Spanned<String>> {
                     let flag_name = &flag.name.as_ref()?.0;
-                    let info = bazel_flags.get_by_invocation(flag_name)?;
+                    let (_, info) = bazel_flags.get_by_invocation(flag_name)?;
                     if flag.value.is_some() {
                         return flag.value.clone();
                     } else if info.requires_value() {
@@ -321,6 +338,7 @@ fn test_flags() {
     assert_eq!(
         preemptible_info
             .unwrap()
+            .1
             .commands
             .iter()
             .map(|n| n.to_string())
@@ -330,8 +348,16 @@ fn test_flags() {
 
     // Supports both short and long forms
     assert_eq!(
-        flags.get_by_invocation("-k"),
-        flags.get_by_invocation("--keep_going")
+        flags.get_by_invocation("-k").unwrap().0,
+        FlagLookupType::Abbreviation
+    );
+    assert_eq!(
+        flags.get_by_invocation("--keep_going").unwrap().0,
+        FlagLookupType::Normal
+    );
+    assert_eq!(
+        flags.get_by_invocation("-k").unwrap().1,
+        flags.get_by_invocation("--keep_going").unwrap().1
     );
 
     // The `remote_cache` is valid for at least one command. Hence, it should be in `common`.

src/bazel_flags_proto.rs

@@ -38,10 +38,16 @@ pub struct FlagInfo {
     /// e.g. --jobs=100.
     #[prost(bool, optional, tag = "10")]
     pub requires_value: ::core::option::Option<bool>,
+    // The old, deprecated name for this option, without leading dashes.
+    // TODO: Fix the tag number after the upstream Bazel change got merged.
+    // See https://github.com/bazelbuild/bazel/pull/25169
+    #[prost(string, optional, tag = "99999")]
+    pub old_name: Option<::prost::alloc::string::String>,
     /// EXTENSION: List of Bazel versions this flag applies to
     #[prost(string, repeated, tag = "999")]
     pub bazel_versions: ::prost::alloc::vec::Vec<::prost::alloc::string::String>,
 }
+
 #[derive(Clone, PartialEq, ::prost::Message)]
 pub struct FlagCollection {
     #[prost(message, repeated, tag = "1")]

src/diagnostic.rs

@@ -6,7 +6,9 @@ use ropey::Rope;
 use tower_lsp::lsp_types::{Diagnostic, DiagnosticSeverity, DiagnosticTag};
 
 use crate::{
-    bazel_flags::BazelFlags, file_utils::resolve_bazelrc_path, lsp_utils::range_to_lsp,
+    bazel_flags::{BazelFlags, FlagLookupType},
+    file_utils::resolve_bazelrc_path,
+    lsp_utils::range_to_lsp,
     parser::Line,
 };
 
@@ -62,7 +64,9 @@ fn diagnostics_for_flags(rope: &Rope, line: &Line, bazel_flags: &BazelFlags) ->
                 .any(|prefix| name.0.starts_with(prefix))
             {
                 // Don't diagnose custom settings at all
-            } else if let Some(flag_description) = bazel_flags.get_by_invocation(&name.0) {
+            } else if let Some((lookup_type, flag_description)) =
+                bazel_flags.get_by_invocation(&name.0)
+            {
                 // Diagnose flags used on the wrong command
                 if !flag_description.supports_command(command) {
                     diagnostics.push(Diagnostic::new_simple(
@@ -84,7 +88,27 @@ fn diagnostics_for_flags(rope: &Rope, line: &Line, bazel_flags: &BazelFlags) ->
                         range: range_to_lsp(rope, &name.1).unwrap(),
                         message: format!("The flag {:?} is a no-op.", name.0),
                         severity: Some(DiagnosticSeverity::WARNING),
+                        ..Default::default()
+                    });
+                } else if lookup_type == FlagLookupType::OldName {
+                    diagnostics.push(Diagnostic {
+                        range: range_to_lsp(rope, &name.1).unwrap(),
+                        message: format!(
+                            "The flag {:?} was renamed to \"--{}\".",
+                            name.0, flag_description.name
+                        ),
                         tags: Some(vec![DiagnosticTag::DEPRECATED]),
+                        severity: Some(DiagnosticSeverity::WARNING),
+                        ..Default::default()
+                    });
+                } else if lookup_type == FlagLookupType::Abbreviation {
+                    diagnostics.push(Diagnostic {
+                        range: range_to_lsp(rope, &name.1).unwrap(),
+                        message: format!(
+                            "Use the full name {:?} instead of its abbreviation.",
+                            flag_description.name
+                        ),
+                        severity: Some(DiagnosticSeverity::WARNING),
                         ..Default::default()
                     });
                 }
@@ -351,6 +375,11 @@ fn test_diagnose_flags() {
         diagnose_string("common --incompatible_override_toolchain_transition"),
         vec!["The flag \"--incompatible_override_toolchain_transition\" is a no-op."]
     );
+    // Diagnose abbreviated flag names
+    assert_eq!(
+        diagnose_string("build -k"),
+        vec!["Use the full name \"keep_going\" instead of its abbreviation."]
+    );
 
     // Don't diagnose custom flags
     assert_eq!(

src/language_server.rs

@@ -298,7 +298,7 @@ impl LanguageServer for Backend {
                 IndexEntryKind::FlagValue(flag_nr) | IndexEntryKind::FlagName(flag_nr) => {
                     let line = &doc.indexed_lines.lines[*line_nr];
                     let flag_name = &line.flags.get(*flag_nr)?.name.as_ref()?.0;
-                    let flag_info = self.bazel_flags.get_by_invocation(flag_name)?;
+                    let (_, flag_info) = self.bazel_flags.get_by_invocation(flag_name)?;
                     let content = flag_info.get_documentation_markdown();
                     let contents = HoverContents::Scalar(MarkedString::String(content));
                     Some(Hover {