Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Patch fix #1109 #1346

Merged
merged 2 commits into from
Feb 19, 2020
Merged

Patch fix #1109 #1346

merged 2 commits into from
Feb 19, 2020

Conversation

jackkoenig
Copy link
Contributor

This isn't a full fix, but it's an improvement of the status quo we've had for months.

Given something like:

import chisel3._

class MyModule extends RawModule {
  def func(): Unit = {
    val port = IO(Output(UInt(8.W))) //.suggestName("port")
    port := 3.U
  }
  func()
}

object MyMain extends App {
  println(chisel3.Driver.emit(() => new MyModule))
}

Currently, you get the following error:

[error] Exception in thread "main" java.lang.ClassCastException: chisel3.internal.firrtl.Ref cannot be cast to chisel3.internal.firrtl.ModuleIO

Pretty worthless. This PR changes the error to

[info] [error] RawModule.scala:53: Unable to name port UInt<8>(IO <UNNAMED> in MyModule) in MyModule@a67c67e, try making it a public field of the Module in class chisel3.RawModule

It does not pinpoint the error so I'm not going to call this a full fix, but at least it tells you something.

A better solution would come from adding source locators to ports such that we could then report the line. I think this is helpful enough to be worth merging without having to adopt that larger project.

Related issue: #1109

Type of change: bug fix

Impact: no functional change

Development Phase: implementation

Release Notes
Patch #1109 giving a better error message

@jackkoenig jackkoenig added this to the 3.2.X milestone Feb 19, 2020
@jackkoenig jackkoenig requested a review from a team as a code owner February 19, 2020 02:07
@jackkoenig
Copy link
Contributor Author

jackkoenig commented Feb 19, 2020

h/t @ekiwi for reminding me of this lingering issue

@jackkoenig jackkoenig added the Bugfix Fixes a bug, will be included in release notes label Feb 19, 2020
Copy link
Contributor

@ekiwi ekiwi left a comment

Choose a reason for hiding this comment

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

Cool! Getting an actual error report instead of throwing an exception while trying to error report is an important improvement!

@jackkoenig jackkoenig added the Please Merge Accepted PRs that are ready to be merged. Useful when waiting on CI. label Feb 19, 2020
@mergify mergify bot merged commit aac6e17 into master Feb 19, 2020
mergify bot pushed a commit that referenced this pull request Feb 19, 2020
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
(cherry picked from commit aac6e17)
@mergify mergify bot mentioned this pull request Feb 19, 2020
@mergify mergify bot added the Backported This PR has been backported label Feb 19, 2020
mergify bot added a commit that referenced this pull request Feb 19, 2020
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
(cherry picked from commit aac6e17)

Co-authored-by: Jack Koenig <jack.koenig3@gmail.com>
@jackkoenig jackkoenig deleted the patch-fix-1109 branch June 1, 2020 19:05
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Backported This PR has been backported Bugfix Fixes a bug, will be included in release notes Please Merge Accepted PRs that are ready to be merged. Useful when waiting on CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants