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

not callling Rf_inherits(sexp if sexp is nullptr #12307

Merged
merged 1 commit into from
Nov 8, 2022
Merged

not callling Rf_inherits(sexp if sexp is nullptr #12307

merged 1 commit into from
Nov 8, 2022

Conversation

romainfrancois
Copy link
Contributor

Intent

addresses #12296

Approach

careful not to call Rf_inherits() on a nullptr.

when the object is not found in the environment, findInNamedEnvir() returns a nullptr:

  • still emit a client event to close the View
  • don't try to call View again

Automated Tests

Indicate whether this change includes unit tests or integration tests, or link a work item or pull request to add those tests to another repo. If the change cannot or will not be covered by a test, indicate why.

QA Notes

Add additional information for QA on how to validate the change, paying special attention to the level of risk, adjacent areas that could be affected by the change, and any important contextual information not present in the linked issues.

Checklist

  • If this PR adds a new feature, or fixes a bug in a previously released version, it includes an entry in NEWS.md
  • If this PR adds or changes UI, the updated UI meets accessibility standards
  • A reviewer is assigned to this PR (if unsure who to assign, check Area Owners list)
  • This PR passes all local unit tests

@romainfrancois
Copy link
Contributor Author

I think this can go in main but happy to move it to cherry-blossom too.

romainfrancois
romainfrancois commented Nov 8, 2022
View reviewed changes
src/cpp/session/modules/data/DataViewer.cpp
@@ -982,15 +982,14 @@ void onDetectChanges(module_context::ChangeSource source)
// clear working data for the object
r::exec::RFunction(".rs.removeWorkingData", i->first).call();

if (Rf_inherits(sexp, "data.frame"))
if (sexp != nullptr && Rf_inherits(sexp, "data.frame"))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rf_inherits(<nullptr>, "data.frame") was the issue

romainfrancois
romainfrancois commented Nov 8, 2022
View reviewed changes
src/cpp/session/modules/data/DataViewer.cpp
.addParam(symbol)
.call(env, true);
if (error)
SEXP env = getNamedEnvir(i->second.envName);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

getNamedEnvir() may also return a nullptr so careful there too.

kevinushey
kevinushey approved these changes Nov 8, 2022
View reviewed changes
Copy link
Contributor

@kevinushey kevinushey left a comment

Choose a reason for hiding this comment

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

LGTM!

@kevinushey kevinushey merged commit 7424282 into main Nov 8, 2022
@kevinushey kevinushey deleted the fix/dataviewer_nullptr_12296 branch November 8, 2022 16:56
jgutman pushed a commit that referenced this pull request Nov 17, 2022
@romainfrancois @jgutman
not callling Rf_inherits(sexp if sexp is nullptr (#12307)
16748b3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kevinushey kevinushey kevinushey approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@romainfrancois @kevinushey