New dataflow summary format #152
Conversation
analysis/dataflow/contracts.go
Outdated
|
|
||
| // IContract is a contract for an interface: it applies to each implmentation of the | ||
| // interface's methods | ||
| type IContract struct { |
There was a problem hiding this comment.
nit: the name IContract makes this seem like an interface type (at least from a dotNET naming convention perspective). I think spelling out InterfaceContract or IfaceContract isn't too long and will make the name more clear
analysis/dataflow/contracts.go
Outdated
| "golang.org/x/tools/go/ssa" | ||
| ) | ||
|
|
||
| // RawContract is the type of contracts for serialization/deserialization that gets compiled | ||
| // into either an interface or a function contract | ||
| type RawContract struct { |
There was a problem hiding this comment.
nit: Can this be private somehow so the implementation details of parsing a contract don't get leaked to the caller?
analysis/dataflow/contracts.go
Outdated
| } | ||
|
|
||
| // FContract is a function contract, it applies only to that set of function implementations | ||
| type FContract struct { |
There was a problem hiding this comment.
nit: I think spelling out FunctionContract isn't that long and will help with readability. Alternatively Fn or FuncContract could work
| data, err := loadRawIndexedFormat(fileName) | ||
| if err == nil { | ||
| // Loaded successfully using the legacy (with indices) format; return | ||
| return data, nil |
There was a problem hiding this comment.
Should we print a warning here that the user should upgrade to the new string-based format?
There was a problem hiding this comment.
Yes, but we'll need to add something in the documentation before doing that. Let's wait a little until we have a stable new format.
| return nil, fmt.Errorf("receiver %s is set, but method is empty", s.Receiver) | ||
| } | ||
| // Can't have a function and a method | ||
| if s.Function != "" && s.Method != "" { |
There was a problem hiding this comment.
Do we need to distinguish between functions and methods in the summaries file? I think we should just stick with methods because we treat functions as methods when parsing a config.CodeIdentifier
There was a problem hiding this comment.
In the spirit of pushing for more constrained formats for specifications like you started doing, I think differentiating is better.
In this case, it allows checking that receiver are only used when methods are used. The function string that is internally used is also generated differently depending on whether it's a method or a function.
| if err == nil { | ||
| t.Error("Expected error") | ||
| } | ||
| expectedSubstrs := []string{ |
There was a problem hiding this comment.
nit: testing error strings is error-prone and will be painful to maintain if we change the error messages in the future. Instead, you can return a public error type like ErrEmptyPackage from ParseSummariesFile and then use errors.Is in the test.
There was a problem hiding this comment.
In this case I'm testing the user interface, I want to avoid someone to inadvertently change the error messages that should be surfaced to the user. I can add a comment in the tests. I don't want to add the error types just for testing; maybe if we need more error handling in the non-testing code then yes.
analysis/summaries/frontend_test.go
Outdated
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
|
|
||
| package summaries |
There was a problem hiding this comment.
nit: having a separate test package summaries_test is nicer because it forces you to only test the package's public API
victornicolet
force-pushed
the
new-dataflow-summary-format
branch
from
5d83ca8 to
9bb1810
Compare
Implements the new format for dataflow summaries described in #131 .
The new format is easier to read and more high-level than the previous format. This PR also adds a lot of validation and error reporting when using the dataflow summaries, making it easier to get them right.