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.

Sign up for GitHub

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

Jump to bottom

Make some usages of suggestName runtime deprecated #2556

Open
wants to merge 30 commits into
base: main
Choose a base branch
from
Open

Make some usages of suggestName runtime deprecated #2556

wants to merge 30 commits into from

Conversation

mwachs5
Copy link
Contributor

@mwachs5 mwachs5 commented Jun 2, 2022 •
edited
Loading

Contributor Checklist

  • Did you add Scaladoc to every public function/method?
  • Did you add at least one test demonstrating the PR?
  • Did you delete any extraneous printlns/debugging code?
  • Did you specify the type of improvement?
  • Did you add appropriate documentation in docs/src?
  • Did you state the API impact?
  • Did you specify the code generation impact?
  • Did you request a desired merge strategy?
  • Did you add text to be included in the Release Notes for this change?

Type of Improvement

  • code cleanup

API Impact

This runtime deprecates some usages of the suggestName API and warns that they will be turned into errors in the future. Specifically, you can no longer:

  • call suggestName on the same HasId more than once
  • call suggestName after containing Module close
  • call suggestName with the same value that the prefix (autoSeed) would have given it
  • call suggestName outside of a builder context
  • Call suggestName on something that is not actually nameable (such as a subfield of an aggregate)

Backend Code Generation Impact

No impact on output verilog, though people avoiding these runtimeDeprecations would see them.

Desired Merge Strategy

  • Squash: The PR will be squashed and merged (choose this if you have no preference.

Release Notes

Some usages of suggestName are runtime deprecated or made an error

Reviewer Checklist (only modified by reviewer)

  • Did you add the appropriate labels?
  • Did you mark the proper milestone (Bug fix: 3.4.x, [small] API extension: 3.5.x, API modification or big change: 3.6.0)?
  • Did you review?
  • Did you check whether all relevant Contributor checkboxes have been checked?
  • Did you do one of the following when ready to merge:
    • Squash: You/ the contributor Enable auto-merge (squash), clean up the commit message, and label with Please Merge.
    • Merge: Ensure that contributor has cleaned up their commit history, then merge with Create a merge commit.

@mwachs5 mwachs5 added this to the 3.5.x milestone Jun 2, 2022
@mwachs5
Copy link
Contributor Author

mwachs5 commented Jun 2, 2022

Opening this as draft since I am still working on the tests, which are revealing to me that there are myriad usages of suggestName inside the chisel internals itself that I was not aware of

@mwachs5 mwachs5 force-pushed the suggest-name-errors branch 2 times, most recently from 936b66e to 9b6e3fe Compare June 3, 2022 09:57
@mwachs5 mwachs5 marked this pull request as ready for review June 3, 2022 09:59
@mwachs5
Copy link
Contributor Author

mwachs5 commented Jun 3, 2022 •
edited
Loading

Note this currently comments out reflective naming as it kept tripping up the "you are suggesting the same name the plugin would have given you" test

@mwachs5 mwachs5 changed the title [WIP] Make some usages of suggestName runtime deprecated Make some usages of suggestName runtime deprecated Jun 3, 2022
@jackkoenig jackkoenig mentioned this pull request Jun 3, 2022
Merged
16 tasks
@mwachs5
Copy link
Contributor Author

mwachs5 commented Jun 19, 2022

This has changed in that I now am checking that thing-you-are-suggestNaming is actually canBeNamed (brought this back from #2561 ), and no longer checking "has name already been used before suggestname", because i'm not sure the suggestion for that given that we don't want to waste space on _computedName... probably simpler to just check if we have already closed the module

@mwachs5 mwachs5 force-pushed the suggest-name-errors branch from 062bb28 to 1cd71a2 Compare June 27, 2022 20:46
mwachs5 added 10 commits June 28, 2022 12:52
@mwachs5
suggestName: add some runtime deprecations
b6addc7 
* suggestName -- add some runtime deprecations
* suggestName -- make it an error to call suggestName outside of Builder Context
* SuggestName: create an internal version and use it internally to avoid runtime deprecations
* Added a SuggestNameSpec to test suggestName behavior
@mwachs5
no more _computedName
08543f3
@mwachs5
post merge cleanups
236a4fd
@mwachs5
Deleted a bunch of chiselName and TransitName to get tests to pass be…
eaedd38 
…cause they are hitting the 'you get the same name as the prefix' warnings
@mwachs5
Add back the check for suggestName after module close
87f0e41
@mwachs5
Add another case that we can name IOs
f0d2499
@mwachs5
scalafmt
a175c1d
@mwachs5
scalafmt
aceaae4
@mwachs5
Add back the @chiselName
3918123
@mwachs5
SuggestNameSpec: use something other than Queue to avoid issues with …
dbb9078 
…chiselName
@mwachs5 mwachs5 force-pushed the suggest-name-errors branch from 1cd71a2 to dbb9078 Compare June 28, 2022 20:00
jackkoenig
jackkoenig requested changes Jun 30, 2022
View reviewed changes
Copy link
Contributor

@jackkoenig jackkoenig left a comment

Choose a reason for hiding this comment

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

Some minor suggestions, and 1 problem we should sort out (the error message for the suggestName matching the automatically derived name).

core/src/main/scala/chisel3/internal/Builder.scala Outdated Show resolved Hide resolved
src/main/scala/chisel3/util/Decoupled.scala Outdated Show resolved Hide resolved
core/src/main/scala/chisel3/internal/Builder.scala Show resolved Hide resolved
src/test/scala/chiselTests/naming/SuggestNameSpec.scala Outdated Show resolved Hide resolved
core/src/main/scala/chisel3/internal/Builder.scala Outdated Show resolved Hide resolved
src/test/scala/chiselTests/naming/SuggestNameSpec.scala Outdated Show resolved Hide resolved
core/src/main/scala/chisel3/internal/Builder.scala Outdated Show resolved Hide resolved
src/test/scala/chiselTests/naming/SuggestNameSpec.scala Show resolved Hide resolved
core/src/main/scala/chisel3/internal/Builder.scala Outdated Show resolved Hide resolved
core/src/main/scala/chisel3/internal/Builder.scala Outdated Show resolved Hide resolved
@mwachs5
Copy link
Contributor Author

mwachs5 commented Jul 2, 2022

I think I've incorporated all the feedback

jackkoenig pushed a commit that referenced this pull request Feb 28, 2023
@scala-steward @mergify
Update sbt-mima-plugin to 1.1.1 (#2556)
9c85b3c 
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jackkoenig jackkoenig jackkoenig requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
3.5.x
Development

Successfully merging this pull request may close these issues.
2 participants
@mwachs5 @jackkoenig