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

[WIP] Feat(CompilerBase-Error): Add 'One-Sentence', text rendering and error handler to CompilerError. #136

Closed
wants to merge 19 commits into from

Conversation

zong-zhe
Copy link
Contributor

@zong-zhe zong-zhe commented Aug 5, 2022

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

  • N
  • Y

fix #115

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

kclvm/compiler_base

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

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Performance regression: Consumes more CPU
  • Performance regression: Consumes more Memory
  • Other
  1. In /kclvm/compiler_base/diagnostic/, the basic structure of One-Sentence is implemented in compiler-base-diagnostic, which includes 'Pendant', 'Sentence' and 'Diagnostic'.
  2. In /kclvm/compiler_base/style/, the basic structure of text-rendering is implemented in compiler-base-style, which includes 'Style', and 'StyleBuffer'.
  3. In /kclvm/compiler_base/macros/, Macro attributes 'DiagnosticBuilderMacro' is created to support generation of 'Diagnostic' from 'error/warning structs'.

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

The unit test cases in
/kclvm/compiler_base/diagnostic/src/tests.rs
/kclvm/compiler_base/style/src/tests.rs

6. Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

None

@zong-zhe zong-zhe added enhancement New feature or request error-handling Issues or PRs related to kcl error handling labels Aug 5, 2022
@zong-zhe zong-zhe added this to the v0.4.3 Release milestone Aug 5, 2022
@zong-zhe zong-zhe requested review from chai2010, Peefy, ldxdl, He1pa and a team August 5, 2022 01:36
@zong-zhe zong-zhe self-assigned this Aug 5, 2022
@coveralls
Copy link
Collaborator

coveralls commented Aug 5, 2022

Pull Request Test Coverage Report for Build 2817522474

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 51 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.07%) to 59.551%

Files with Coverage Reduction New Missed Lines %
kclvm/sema/src/resolver/attr.rs 12 64.62%
kclvm/sema/src/resolver/node.rs 39 75.23%
Totals Coverage Status
Change from base Build 2780151826: 0.07%
Covered Lines: 22365
Relevant Lines: 37556

💛 - Coveralls

Copy link
Contributor

@Peefy Peefy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pls merge your 17 commits+

@Peefy
Copy link
Contributor

Peefy commented Aug 5, 2022

Delete KCL related content in CompilerBase, considering generality

@@ -0,0 +1,108 @@
//! 'Style' is responsible for providing 'Shader' for text color rendering.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest this package is available as a 3rdparty for compiler_base

///
/// e.g. an error diagnostic.
/// error[E0999]: oh no! this is an error!
/// --> mycode.k:3:5
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove the *.k

/// Sentence 1: error[E0999]: oh no! this is an error!
///
/// Sentence 2:
/// --> mycode.rs:3:5
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove the *.rs


// before
//
// KCL Complier Error[E2L28] : Unique key error
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove KCL

/// error: this is an error!
/// warning[W0011]: this is an warning!
/// note: this is note.
/// KCL error[E1000]: this is a KCL error!
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove KCL

/// A Position is valid if the line number is > 0.
/// The line and column are both 1 based.
#[derive(PartialEq, Clone, Eq, Hash, Debug, Default)]
pub struct Position {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use Span and SourceMap instead of Position to calculate the error Postion.

@@ -0,0 +1,11 @@
schema Person:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove KCL tests.

@@ -0,0 +1,14 @@
[package]
name = "compiler-base-diagnostic"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should compiler-base-diagnostic belongs to compiler-base-error ?

@@ -52,5 +52,8 @@ members = [
"sema",
"span",
"tools",
"version"
"version",
"compiler_base",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

compiler_base is not one of kclvm members.

use std::panic;
use compiler_base_diagnostic::{emitter::{Emitter, EmitterWriter}, DiagnosticBuilder};

pub struct ErrHandler {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How can I use ErrHandler? and it should be part of the error package?

@Peefy
Copy link
Contributor

Peefy commented Aug 5, 2022

Important things:

  • All error submodules should belong to the compiler-base-errors crate.
  • Need to describe how to use Handler, how to define user own errors, and what the default errors are in lib.rs of compiler-base-errors crate.
  • It should not contain anything KCL related.

"version"
"version",
"compiler_base",
"compiler_base/error",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/error/errors/

@Peefy Peefy changed the title Feat(CompilerBase-Error): Add 'One-Sentence', text rendering and error handler to CompilerError. [WIP] Feat(CompilerBase-Error): Add 'One-Sentence', text rendering and error handler to CompilerError. Aug 9, 2022
@Peefy
Copy link
Contributor

Peefy commented Aug 17, 2022

Split large WIP PRs into smaller ones

@Peefy Peefy closed this Aug 17, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Aug 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request error-handling Issues or PRs related to kcl error handling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature] An error handling kit: CompilerBase-Error
4 participants