Skip to content

fix: consider code with existing errors in code suggestions

What does this merge request do and why?

For some complex/large code files, e.g.: spec/models/commit_spec.rb or app/models/project.rb:

  • Codestral seems to misinterpret some of the code blocks in the prefix/content_above_cursor and suggests closing content when it's unnecessary
  • The fix_end_block_errors, which parses the code using treesitter, also seem to "think" that the original code before the suggestion is inserted already has errors

The combination of the above factors mean that Codestral unnecessarily adds closing content, and the fix_end_block_errors cannot resolve it.

This MR adds a new fix_end_block_errors_codestral post-processor that, instead of checking whether the full code with the suggestion has errors, it checks whether there is an increase in the number of errors after the suggestion is inserted to the code.

This does not change fix_end_block_errors for the following reasons:

  • fix_end_block_errors is used by the active Code Completion model (Code Gecko). We do not want any change for Codestral to disrupt Code Gecko behavior. Notably, fix_end_block_errors_codestral will be slower because this parses the code twice.
  • We are expecting more problems of this variety (see #655 (closed)), so having a specific fix_end_block_errors_codestral means we can make future post-processing changes specific to codestral

Example/Illustration of change:

If we have this original code:

def hello(
  [cursor here]
end

With this suggestion:

  puts 'hello'
end

fix_end_block_errors WILL NOT trim out the extra end in the suggestion because the original code has parse errors (the un-closed ().

fix_end_block_errors_codestral SHOULD trim out the extra end because it is not counting the the errors from the original code.

How to set up and validate locally

This problem has been observed in Ruby, so we will test with a Ruby example.

  1. Create a simple Ruby spec file:

    # frozen_string_literal: true
    
    require 'spec_helper'
    
    RSpec.describe "Person" do
      using RSpec::Parameterized::TableSyntax
    
      describe "#fullname" do
        context 'with different name variations' do
          where(:person, :expected_full_name) do
            build(:person, first_name: "Pam", last_name: nil) | "Pam"
            build(:person, first_name: nil, last_name: "Artiaga") | "Artiaga"
            build(:person, first_name: "Pam", last_name: "Artiaga") | "Pam Artiaga"
          end
    
          it 'returns the correct full name' do
          end
        end
      end
    end
  2. On your IDE, open up another large Ruby file. This is to provide more "confusion" to the model since currently opened files are sent as additional context. For example, you can use the (spec/models/commit_spec.rb) in the GitLab Rails monolith.

  3. Type around inside the it 'returns the correct full name' of the spec.

    • if you are on main, you will sometimes get suggestions with unnecessary end content, e.g.:

      Screenshot_2024-10-02_at_17.10.50

    • if you are on this MR's branch (655-remove-closing-content-from-codestral-suggestions), the last end content should be stripped:

      Screenshot_2024-10-02_at_17.12.32

Merge request checklist

  • Tests added for new functionality. If not, please raise an issue to follow up.
  • Documentation added/updated, if needed.

Closes #655 (closed)

Edited by Pam Artiaga

Merge request reports

Loading