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

fixes #17437 - crash where error reporting > 1 #17547

Merged
merged 3 commits into from
Mar 29, 2021
Merged

Conversation

saem
Copy link
Contributor

@saem saem commented Mar 28, 2021

Crash occurs in cases where max error reporting is greater than 1 and an object
constructor error exists in a proc body.

Without this the tracking done in semProcAux will assume construction was
successful and not be able to handled the non-semmed nodes.

Scenarios where this occurs:

  • nimsuggest
  • nim check
  • nim with errorMax > 1

This is also generally the correct direction nudging us towards proper nkError.

fixes #17437

Co-authored-by: Timothee Cour <[email protected]>
@dom96
Copy link
Contributor

dom96 commented Mar 28, 2021

Suggestion: to make it easier to go through notifications, include more detail about the issue in the PR title.

@dom96 dom96 changed the title fixes #17437 fixes #17437 (nimsuggest crash) Mar 28, 2021
@saem
Copy link
Contributor Author

saem commented Mar 28, 2021

Huh, why didn't [skip ci] work?

@saem saem changed the title fixes #17437 (nimsuggest crash) fixes #17437 - crash where error reporting > 1 Mar 28, 2021
@timotheecour
Copy link
Member

timotheecour commented Mar 28, 2021

Huh, why didn't [skip ci] work?

  • why CI pipelines did it not skip?
    we have:;
.github/workflows/ci_docs.yml:34:105:      !contains(format('{0} {1}', github.event.head_commit.message, github.event.pull_request.title), '[skip ci]')
.github/workflows/ci_packages.yml:7:105:      !contains(format('{0} {1}', github.event.head_commit.message, github.event.pull_request.title), '[skip ci]')
doc/contributing.rst:464:39:   documentation only changes), add `[ci skip]` to your commit message title.

(ci skip vs skip ci)

@saem
Copy link
Contributor Author

saem commented Mar 28, 2021

Thanks, @timotheecour.

Hmm, I wonder if we could do something like have one pipeline stage before everything, which can detect if the change is meaningful and conservatively do a skip? (just seeding a thought to think about in the background, no action needed)

@timotheecour
Copy link
Member

Hmm, I wonder if we could

@saem in the spirit of "1 topic, 1 issue", let's move this discussion to timotheecour#675

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Nimsuggest crash
4 participants