feat: add completion item for string union types

[shruti2522]

1. Does this PR affect any open issues?(Y/N) and add issue references (e.g. "fix #123", "re #123".):

• N
• Y
  fix [Enhancement] Add completion item for the string union types. #1391

2. What is the scope of this PR (e.g. component or file name):

kclvm/tools/src/LSP/src/completion.rs

3. Provide a description of the PR(e.g. more details, effects, motivations or doc link):

Screencast.from.2024-06-06.21-56-13.webm

• Affects user behaviors
• Contains syntax changes
• Contains variable changes
• Contains experimental features
• Performance regression: Consumes more CPU
• Performance regression: Consumes more Memory
• Other

4. Are there any breaking changes?(Y/N) and describe the breaking changes(e.g. more details, motivations or doc link):

• N
• Y

5. Are there test cases for these changes?(Y/N) select and add more details, references or doc links:

• Unit test
• Integration test
• Benchmark (add benchmark stats below)
• Manual test (add detailed scripts or steps below)
• Other

[Peefy]

Hello @shruti2522

Please note that the position of <cursor> I mentioned in the issue is a completion feature of dot ".", not a completion of the attribute value list.

cc @He1pa Could you help review it?

[shruti2522]

Hello @shruti2522

Please note that the position of <cursor> I mentioned in the issue is a completion feature of dot ".", not a completion of the attribute value list.

cc @He1pa Could you help review it?

Okay got it @Peefy , I will push the code for dot completion. Should I undo the attribute completion logic here?

[coveralls]

Pull Request Test Coverage Report for Build 9476907236

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

• For more information on this, see Tracking coverage changes with pull request builds.
• To avoid this issue with future PRs, see these Recommended CI Configurations.
• For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

• 0 of 6 (0.0%) changed or added relevant lines in 1 file are covered.
• 3 unchanged lines in 1 file lost coverage.
• Overall coverage remained the same at 71.075%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
kclvm/sema/src/core/symbol.rs	0	6	0.0%

Files with Coverage Reduction	New Missed Lines	%
kclvm/sema/src/core/symbol.rs	3	49.52%

Totals
Change from base Build 9394743478:	0.0%
Covered Lines:	55030
Relevant Lines:	77425

---

💛 - Coveralls

[Peefy]

Okay got it @Peefy , I will push the code for dot completion. Should I undo the attribute completion logic here?

Sure. Undo it

[shruti2522]

Hello @shruti2522

Please note that the position of <cursor> I mentioned in the issue is a completion feature of dot ".", not a completion of the attribute value list.

Done @Peefy

[Peefy]

Hello. You can just add symbol complete items here for the string lit type 😄: https://github.com/kcl-lang/kcl/blob/main/kclvm/sema/src/core/symbol.rs#L385

[shruti2522]

Hello. You can just add symbol complete items here for the string lit type 😄: https://github.com/kcl-lang/kcl/blob/main/kclvm/sema/src/core/symbol.rs#L385

Hello @Peefy, I tried using get_type_all_attribute earlier, but it needs the name argument, which requires the string symbol def to be defined first. So, I used get_type_symbol instead because it doesn't require the name arg.

[Peefy]

Hello. You can just add symbol complete items here for the string lit type 😄: https://github.com/kcl-lang/kcl/blob/main/kclvm/sema/src/core/symbol.rs#L385

Hello @Peefy, I tried using get_type_all_attribute earlier, but it needs the name argument, which requires the string symbol def to be defined first. So, I used get_type_symbol instead because it doesn't require the name arg.

cc @He1pa Could you help give more comments?

[shruti2522]

Do I need to make any changes to the current approach @Peefy @He1pa ?

[Peefy]

Do I need to make any changes to the current approach @Peefy @He1pa ?

Yes.

[shruti2522]

Hello, I attempted to integrate the get_type_all_attribute function. In my first approach, I extracted the symbol definition for the union type and passed the definition name to the argument. In the second approach, I passed an empty string to the name argument. However, in both cases, failure occurs after one successful run. Could you please review the code? @Peefy

1. union def method

                    let sema_info = def.get_sema_info();
                    if let Some(ty) = &sema_info.ty {
                        let ty_symbl_refs = gs
                            .get_symbols()
                            .get_type_symbol(ty, gs.get_packages().get_module_info(&pos.filename));
                        if let Some(ty_symbl_ref) = ty_symbl_refs {
                            if let Some(ty_symbl_def) = gs.get_symbols().get_symbol(ty_symbl_ref) {
                                if let TypeKind::Union(union_types) = &ty.kind {
                                    for union_ty in union_types {
                                        if let TypeKind::StrLit(_) | TypeKind::Str = &union_ty.kind
                                        {
                                            let str_attrs =
                                                gs.get_symbols().get_type_all_attribute(
                                                    &union_ty,
                                                    &ty_symbl_def.get_name(),
                                                    gs.get_packages()
                                                        .get_module_info(&pos.filename),
                                                );

2. empty str method

                    let sema_info = def.get_sema_info();
                    if let Some(ty) = &sema_info.ty {
                        if let TypeKind::Union(union_types) = &ty.kind {
                            for union_ty in union_types {
                                if let TypeKind::StrLit(_) | TypeKind::Str = &union_ty.kind {
                                    let module_info =
                                        gs.get_packages().get_module_info(&pos.filename);
                                    let str_attrs = gs.get_symbols().get_type_all_attribute(
                                        union_ty,
                                        "",
                                        module_info
                                    );

current behaviour:

Screencast.from.2024-06-11.13-25-37.webm

[Peefy]

cc @He1pa Could you help review it?

[shruti2522]

done @Peefy

Screencast.from.2024-06-12.02-20-01.webm

[He1pa]

Why do we need to get all the symbols in the scope here? I think it is enough to just modify the get_all_attributes part, and the complete function should not be modified.

[shruti2522]

done @He1pa

[shruti2522]

Do I need to make any further changes @Peefy @He1pa ?

[Peefy]

Hello @shruti2522

I think just modifying the symbol.rs file is enough, there is no need to modify the completion.rs file. The correct modification method only requires two steps

1. adding a union type branch to the get_type_symbol function

            TypeKind::Union(types) => {
                if types.iter().all(|ut| {
                    matches!(
                        &ut.kind,
                        TypeKind::StrLit(_) | TypeKind::Str
                    )
                }) {
                    self.get_symbol_by_fully_qualified_name(BUILTIN_STR_PACKAGE)
                } else {
                    None
                }
            }

2. change the get_type_all_attribute function

            TypeKind::StrLit(_) | TypeKind::Str => {
                let mut result = vec![];
                if let Some(symbol_ref) = self.get_type_symbol(ty, module_info) {
                    if let Some(symbol) = self.get_symbol(symbol_ref) {
                        result = symbol.get_all_attributes(self, module_info);
                    }
                }
                result
            }

Besides, you need to add more unit tests for this feature.

[Peefy]

LGTM